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

Make all events go through the DOM #239

Merged
merged 4 commits into from Jul 12, 2022
Merged

Conversation

KittyGiraudel
Copy link
Owner

@KittyGiraudel KittyGiraudel commented Sep 6, 2021

This is a proof of concept that would make all events go through the DOM under the hood. This would mean they would no longer be attached to the dialog instance itself, but on the dialog DOM container instead. This just reduces the amount of code shipped (not by a lot though).

The way this is implemented would likely cause no API breaking change. The object passed to the events would now be a CustomEvent, and accessing the underlying DOM node would require reading event.detail.currentTarget instead of event.currentTarget, so this is a breaking change.

We could also decide to drop .on and .off methods entirely, but it might be good to keep them for convenience. Otherwise the API would be a little awkward (having to read the element from the instance to bind listeners).

@KittyGiraudel
Copy link
Owner Author

@mxmason I would love your opinion on this. :)

@mxmason
Copy link
Contributor

mxmason commented Jul 9, 2022

I think this is a good idea, and I'd be in favor of implementing it in our TypeScript file. I think we can keep on, off, and even fire internally for convenience, but just use DOM events

@KittyGiraudel
Copy link
Owner Author

Agreed. Re-reading that PR the other day, I also thought we should keep the fire method but update its implementation.

@mxmason
Copy link
Contributor

mxmason commented Jul 12, 2022

So. While researching this a bit, I discovered that the CustomEvent constructor isn't actually supported in IE. 😬 Maybe nothing broke 'til now because authors aren't reacting to those events much. Polyfilling the CustomEvent constructor is relatively simple, but does add about ~1kb to the library. I'm fine with either adding it or reconsidering whether to continue supporting IE 11.

@KittyGiraudel
Copy link
Owner Author

Interesting. What do you think?

@mxmason
Copy link
Contributor

mxmason commented Jul 12, 2022

After thinking about it, I’d like to propose that we drop IE11 with version 8 and point authors to 7.x if they need IE support.

@KittyGiraudel
Copy link
Owner Author

I‘m happy with that decision. I think it’s also moving the needle forward. All good with me.

@mxmason
Copy link
Contributor

mxmason commented Jul 12, 2022

Sweet. I'll get to work pulling out the build tooling we no longer need :)

@KittyGiraudel
Copy link
Owner Author

Yay! Thank you. 😊

@KittyGiraudel
Copy link
Owner Author

@mxmason Hiii! ✨ I rebased that PR but have a TypeScript issue I don’t understand. Any clue? 😅
Screenshot 2022-07-12 at 19 30 54

@KittyGiraudel
Copy link
Owner Author

Oh, I think it’s because we can use the EventListener native type now.

@mxmason
Copy link
Contributor

mxmason commented Jul 12, 2022

Ah, you got it! I think this looks good to ship if you're ready!

Edit to add: Should our custom events be cancelable? I think so, just in case the author wants that ability, although I don't know why they would. Also, do we still actually need the event arg for show and hide? What use cases would the author get out of it?

@KittyGiraudel KittyGiraudel merged commit 8364c67 into main Jul 12, 2022
@KittyGiraudel KittyGiraudel deleted the custom-events-no-more branch July 12, 2022 19:37
@chalkygames123
Copy link
Contributor

@KittyGiraudel friendly ping :)

@KittyGiraudel
Copy link
Owner Author

@chalkygames123 Why?

@chalkygames123
Copy link
Contributor

Because @mxmason added some questions in the last comment. Sorry if I missed something or if this has already been resolved in a private discussion 😅

@KittyGiraudel
Copy link
Owner Author

KittyGiraudel commented Jul 23, 2022

Oh, I actually totally missed that edit from EJ. Thank you for pinging me!

Edit to add: Should our custom events be cancelable? I think so, just in case the author wants that ability, although I don't know why they would.

I would say yes. Probably best to be flexible? I’ll issue a commit to deal with that. Edit: 1c15741.

Also, do we still actually need the event arg for show and hide? What use cases would the author get out of it?

I think this is important to know which DOM node triggered the open/close of the dialog. Without the event, we can’t I’m afraid?

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

Successfully merging this pull request may close these issues.

None yet

3 participants