[DataGrid] Don't listen in the capture phase #1156
Conversation
| @@ -199,8 +199,8 @@ export function useEvents(gridRootRef: React.RefObject<HTMLDivElement>, apiRef: | |||
| return () => { | |||
| logger.debug('Clearing all events listeners'); | |||
| 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
oliviertassinari
added
the
component: data grid
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.