Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ModalCloseWatcher should possibly use a promise rather than close event #34

Open
tabatkins opened this issue Feb 26, 2021 · 4 comments
Open

Comments

@tabatkins
Copy link

In https://github.com/slightlyoff/history_api/blob/master/history_and_modals.md the ModalCloseWatcher's primary interaction with the developer is firing either zero or one close events. It never fires multiple close events; once the close event has fired, the object doesn't do anything anymore; and the close event is purely informative, so nothing you do in response can affect the object.

This is the exact interaction pattern that Promises are better suited for. Could the close event instead become a .closed Promise attribute?

(The beforeclose proposed event should stay as it is; it can fire multiple times, you can preventDefault it, etc.)

@domenic
Copy link
Collaborator

domenic commented Apr 9, 2021

I'm torn on this largely because of the symmetry with <dialog>. As you're aware, sadly many places on the platform that could be using promises in this way are still using events. And <dialog> is a prominent one, that we want to stay aligned with.

Maybe we should have both??

@domenic
Copy link
Collaborator

domenic commented Apr 9, 2021

Another issue is that the promise-for-one-time-event thing works best when it's OK to "subscribe" even after the event has been fired. E.g. document.loaded is great as a promise because if you do document.loaded.then(doStuff) after the document has already loaded, then you definitely want to doStuff().

I'm not sure the same thing holds for close signals. If you do watcher.closed.then(doStuff) but the associated component has already been closed, do you think you'd really want to run doStuff()?

@tabatkins
Copy link
Author

I'm torn on this largely because of the symmetry with .

Ah, that's reasonable.

If you do watcher.closed.then(doStuff) but the associated component has already been closed, do you think you'd really want to run doStuff()?

I think, realistically, yes? It's going to depend on the situation, of course, but if you did some setup for the modal (some styling elsewhere on the page, for example) and need to do teardown after it closes, you'd want this to still happen even if you missed the opening.

The opposite is true as well, of course, where you don't want to do something in response to an already-closed dialog. However, leaking an event listener because you registered it after missing the event isn't great (tho probably insignificant...)

It looks like there's no state exposed for whether the CloseWatcher is still pending or already closed; exposing that would at least let code tell whether they should even register for the close event or not, which would satisfy my use-case.

@domenic
Copy link
Collaborator

domenic commented Apr 9, 2021

I'm still noodling on the rest of the discussion, but I want to agree with:

It looks like there's no state exposed for whether the CloseWatcher is still pending or already closed; exposing that would at least let code tell whether they should even register for the close event or not, which would satisfy my use-case.

This seems like a good thing to add independent of the promise vs. event discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants