[DataGrid] Don't listen in the capture phase#1156
[DataGrid] Don't listen in the capture phase#1156oliviertassinari wants to merge 1 commit intomui:masterfrom
Conversation
| api.publishEvent(GRID_UNMOUNT); | ||
| gridRootElem.removeEventListener(GRID_CLICK, onClickHandler, { capture: true }); | ||
| gridRootElem.removeEventListener(GRID_MOUSE_HOVER, onHoverHandler, { capture: true }); | ||
| gridRootElem.removeEventListener(GRID_CLICK, onClickHandler); |
There was a problem hiding this comment.
the event bubble up so if you attach them to the root then they won't fire when you click an element within it.
There was a problem hiding this comment.
I don't understand the reasoning. This argument would be valid if we were listening to an event that doesn't bubble, like the scroll event or focus. click/mouseover bubble.
Regarding the outcome, the demos in https://deploy-preview-1156--material-ui-x.netlify.app/components/data-grid/selection/#data-grid-selection seems to work correctly and the tests are green.
There was a problem hiding this comment.
Before we move forward with #1158, could you report one concrete bug with the current changes? This could then be added as a regression test case for the future. The notion seems important enough so we sync with reality.
There was a problem hiding this comment.
the event bubble up so if you attach them to the root then they won't fire when you click an element within it.
A counterexample that demonstrates this affirmation is false: https://codesandbox.io/s/nifty-bush-brmqw?file=/src/App.tsx
There was a problem hiding this comment.
Yes I agree I got really confused here with the bubbling. Just need to check the params are still ok but I think they should be
There was a problem hiding this comment.
The nice thing about #1158 is that it could use the helper to generate the params...
The bad thing is atm we generate a new function at every render so react.memo is not possible
There was a problem hiding this comment.
Ok, what do you recommend as the best next step?
There was a problem hiding this comment.
I will refactor a bit more and merge #1158 with the comments above
2 participants
Opened it for experimentation. I don't understand this thread: #1025 (comment). I want to see if the CI is green and play with the built demos. Related to #1157.