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

Assorted Event handler patches #36171

Merged
merged 7 commits into from
May 16, 2022
Merged

Assorted Event handler patches #36171

merged 7 commits into from
May 16, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Apr 14, 2022

mostly renaming, to help my eyes, follow the logic and some minor refactoring to separate some logic

@GeoSot GeoSot requested a review from a team as a code owner April 14, 2022 10:06
@GeoSot GeoSot marked this pull request as draft April 14, 2022 10:16
@GeoSot GeoSot marked this pull request as ready for review April 14, 2022 10:54
@GeoSot GeoSot added the squash label Apr 14, 2022
@GeoSot GeoSot force-pushed the gs/patches-on-event-handler branch from 5beca29 to 48c952b Compare April 14, 2022 22:20
@GeoSot
Copy link
Member Author

GeoSot commented Apr 16, 2022

@julien-deramond @louismaximepiton does these changes make sense to you guys? 😷

@GeoSot GeoSot force-pushed the gs/patches-on-event-handler branch from c64c81d to 6241cd3 Compare April 18, 2022 07:27
@julien-deramond
Copy link
Member

@julien-deramond @louismaximepiton does these changes make sense to you guys? mask

I haven't tested the minor refactoring but the new naming allows a better understanding.
Especially for me:

  • getEventgetElementEvents that highlights when you read it the fact that you get multiple events
  • delegationisDelegated is more obvious as well
  • I know some developers don't like the fact to have a function used only one time but IMO hydrateObj makes the code more readable here

Regarding callable I don't know well this JS file so I don't have an opinion on this. I just like that it avoids having originalHandler + handler.

@louismaximepiton
Copy link
Member

@julien-deramond @louismaximepiton does these changes make sense to you guys? 😷

I haven't tested too, but the changes seems good and understandable to me.

Regarding the hydrateObj function, I was wondering if this function could be in the utils file instead of a standalone one here ?

@GeoSot
Copy link
Member Author

GeoSot commented Apr 19, 2022

Regarding the hydrateObj function, I was wondering if this function could be in the utils file instead of a standalone one here ?

Thank you, I 'll keep it in mind, for later usage . For now, I didn't find any other place that needs this functionality

@GeoSot GeoSot force-pushed the gs/patches-on-event-handler branch from 6241cd3 to 6e84bf9 Compare April 21, 2022 19:48
@XhmikosR XhmikosR removed the squash label Apr 22, 2022
@XhmikosR XhmikosR marked this pull request as draft April 22, 2022 03:44
@XhmikosR XhmikosR changed the title Patches on event handler Assorted Event handler patches Apr 22, 2022
@GeoSot GeoSot force-pushed the gs/patches-on-event-handler branch from e84341d to f23f22c Compare April 27, 2022 07:23
@GeoSot GeoSot marked this pull request as ready for review May 3, 2022 09:40
@GeoSot GeoSot force-pushed the gs/patches-on-event-handler branch 2 times, most recently from 4525209 to 75f8327 Compare May 9, 2022 09:25
@mdo mdo force-pushed the gs/patches-on-event-handler branch from 75f8327 to 05ed16b Compare May 16, 2022 00:25
@mdo mdo force-pushed the gs/patches-on-event-handler branch from 2138e95 to 2170b2a Compare May 16, 2022 14:55
@mdo mdo merged commit b5a9567 into main May 16, 2022
@mdo mdo deleted the gs/patches-on-event-handler branch May 16, 2022 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants