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

Deprecate mouseEnter/Leave/Move Ember events #486

Merged
merged 7 commits into from
Jul 19, 2019

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Apr 27, 2019

@simonihmig simonihmig force-pushed the deprecate-mouseenter branch from b1f575c to 321566e Compare April 27, 2019 23:49
@simonihmig simonihmig force-pushed the deprecate-mouseenter branch from 321566e to 37ae192 Compare April 27, 2019 23:52
@simonihmig simonihmig changed the title Deprecate mouseEnter/Leave events in EventDispatcher Deprecate mouseEnter/Leave Ember events Apr 27, 2019
locks
locks previously requested changes Apr 29, 2019
Copy link
Contributor

@locks locks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes :)

@rwjblue
Copy link
Member

rwjblue commented Apr 29, 2019

Thoughts on also adding mousemove to the list to be deprecated? I realize it doesn't have all of the same issues (since it bubbles), but it definitely has the same negative performance impact.

@rwjblue rwjblue self-assigned this Apr 29, 2019
@rwjblue
Copy link
Member

rwjblue commented Apr 29, 2019

Thanks for putting this together @simonihmig!

@simonihmig
Copy link
Contributor Author

simonihmig commented Apr 29, 2019

Thoughts on also adding mousemove to the list to be deprecated?

Yes, I mentioned this under "Alternatives", and personally would be inclined to also include this!

Here's a profiler view when just moving the mouse over an Ember app:

image

The first two scripting executions (mouseout + mouseover) are for mouseenter/leave support, the third is just the normal event listener for mousemove. All these take just a fraction of a millisecond, though at large scale it looks like this: 😕

image

Co-Authored-By: Jessica Jordan <[email protected]>
@rwjblue rwjblue dismissed locks’s stale review May 31, 2019 17:44

addressed

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2019

@simonihmig - Would you mind updating to include mouseover and mousemove? I'm adding this to an upcoming core team meeting to see about advancing...

@simonihmig
Copy link
Contributor Author

Would you mind updating to include mouseover and mousemove?

@rwjblue updated this to include mousemove!

But mouseover is not supported anyway here. It was used for the mouseEnter implementation internally, but does not get dispatched itself, AFAICT.

@simonihmig simonihmig changed the title Deprecate mouseEnter/Leave Ember events Deprecate mouseEnter/Leave/Move Ember events Jun 5, 2019
@rwjblue
Copy link
Member

rwjblue commented Jun 5, 2019

Ah, sorry for the confusion thank you @simonihmig!

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this a bit during a recent weekly call, and would love to see some tweaks to the examples.

Co-Authored-By: Clemens M�ller <[email protected]>
@rwjblue
Copy link
Member

rwjblue commented Jul 12, 2019

Discussed this at today's team meeting, and are 👍 on moving this into final comment period.

@simonihmig
Copy link
Contributor Author

Updated this to

  • show deprecations at creation time rather than when dispatching
  • refer to separate deprecation guides for Ember.Component vs. {{action}}
  • include Component API docs for references to events that need to be removed

@pzuraq
Copy link
Contributor

pzuraq commented Jul 19, 2019

At today's core team meeting, we have decided to move forward with this RFC. 👍

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

Successfully merging this pull request may close these issues.

7 participants