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

Change ondrag event interface back to DragEvent #1254

Merged
merged 1 commit into from
May 21, 2020

Conversation

siku2
Copy link
Member

@siku2 siku2 commented May 21, 2020

My previous PR #1244 accidentally replaced a few of the event handler types with less specific types. Most notably it changed the event interfaces for the ondrag handlers from DragEvent to MouseEvent.

The only other place where a previous type was replaced is onscroll which had its interface changed from UiEvent to Event. This struck me as odd because I explicitly remember copying the onscroll event type. So I looked into it and there's a lot of conflicting information on which interface it takes.
The MDN page for the global event handler claims that "The function receives a UIEvent object as its sole argument" but the page for the scroll event lists Event as the interface.
I remembered that there's already a source of types for these events out there in the wild: TypeScript!
I found a PR which changes the interface from UiEvent to Event so I decided to leave it as Event.

I went through the new additions as well and I found another event handler with conflicting information. onresize also has multiple mentions of using UiEvent but the CSSOM (which is what was used in the previously mentioned TypeScript) mentions that onresize also takes the Event interface.
The UiEvent is still being re-exported by Yew so that it's easy to cast to it if necessary.

This is all incredibly confusing and it takes a lot of time to research.
It is very possible - even likely - that other mistakes slipped in and I'm very sorry for that.
At least it should be possible to use JsCast so even if an event handler has the wrong type it should be possible to use it.

@jstarry
Copy link
Member

jstarry commented May 21, 2020

@siku2 thanks for your thoroughness on this, I appreciate that you took the time to research so much.

That being said, I think it's important to remember that we are all contributing to Yew because we enjoy it! If you make a small mistake, don't sweat it 😄we can't be perfect

If there is a bug, someone in the community will likely bring it up and may even fix it themselves before you get the chance 😉 Of course we all take pride in our work and want it to be high quality. I just want to assure you that your contributions have been great and I look forward to more!

@jstarry jstarry merged commit 56979ca into yewstack:master May 21, 2020
@siku2
Copy link
Member Author

siku2 commented May 21, 2020

@jstarry Thank you for saying this. I always feel incredibly guilty and responsible when there's a mistake in a submission of mine - probably a bit more than would be healthy. It helps a lot to hear that you're so understanding ❤️

@siku2 siku2 deleted the fix-event-listeners branch May 21, 2020 16:38
@webbrandon
Copy link

👏

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.

3 participants