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

[DataGrid] Use event.defaultMuiPrevented to prevent the default behavior #2179

Merged
merged 9 commits into from
Jul 28, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Jul 21, 2021

Breaking changes

  • [DataGrid] Replace event.stopPropagation() with event.defaultMuiPrevented = true in all callback props.

    Previously, calling event.stopPropagation would stop the propagation of an event and also prevent its the default behavior from running. Now, these two actions can be controlled independently:

    • Set event.defaultMuiPrevented to true to prevent the default action of an event

    • Call event.stopPropagation to stop an event from propagating to parent elements

    <DataGrid
      onCellDoubleClick={(params, event) => {
    -   event.stopPropagation();
    +   event.defaultMuiPrevented = true;
      }}
    />
  • [XGrid] Replace event.stopPropagation() with event.defaultMuiPrevented = true in all event listeners.

    apiRef.current.subscribeEvent('cellDoubleClick', (params, event) => {
    - event.stopPropagation();
    + event.defaultMuiPrevented = true;
    });

    For more information, check this page.


Closes #1403

Preview: https://deploy-preview-2179--material-ui-x.netlify.app/components/data-grid/events/#disabling-the-default-behavior

TODO

@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jul 21, 2021
@m4theushw m4theushw force-pushed the prevent-default-behavior branch from 01ce81c to bed8472 Compare July 21, 2021 01:18
@m4theushw m4theushw force-pushed the prevent-default-behavior branch 3 times, most recently from 66c07f1 to f361c79 Compare July 22, 2021 21:32
@m4theushw m4theushw force-pushed the prevent-default-behavior branch from f361c79 to d2030d0 Compare July 22, 2021 21:43
@m4theushw m4theushw marked this pull request as ready for review July 22, 2021 22:19
docs/src/pages/components/data-grid/events/events.md Outdated Show resolved Hide resolved
@@ -206,139 +206,166 @@ export interface GridOptions {
/**
* Callback fired when the edit cell value changes.
* @param {GridEditCellPropsParams} params With all properties from [[GridEditCellPropsParams]].
* @param {React.SyntheticEvent} event The event that caused this prop to be called.
* @param {MuiEvent} event The event that caused this prop to be called.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {MuiEvent} event The event that caused this prop to be called.
* @param {MuiEvent<React.SyntheticEvent>} event The event that caused this prop to be called.

? x the other


I would personally be fine if I had to do this in my codebase.:

(event as any).defaultMuiPrevented = true;
Suggested change
* @param {MuiEvent} event The event that caused this prop to be called.
* @param {React.SyntheticEvent} event The event that caused this prop to be called.

but I can understand if we don't like this idea 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

We started to use this MuiEvent. Would be nice to keep it, because of the special property. Sometimes using it can lead to terrible typings when the event can be fired both by a synthetic event as well as by a native event, e.g. MuiEvent<React.MouseEvent | DocumentEventMap['click']>.

packages/storybook/src/stories/grid-rows.stories.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/data-grid/events/events.md Outdated Show resolved Hide resolved
@@ -8,7 +8,7 @@ title: Data Grid - Events

## Subscribing to events
Copy link
Member Author

Choose a reason for hiding this comment

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

This page is not good. My idea is to remove the Pro badge from the page and move it to this section only. Then, add a new section only for the event props, e.g. onCellClick, explaining that they are like subscribing to events but with less granularity. For the catalog of events, I want to improve the docs generator to include a third column with the event prop that is mapped to each event name. I think that with our convention GRID_CELL_CLICK -> onCellClick I can discover the mappings. cc @oliviertassinari

params: any,
event: MuiEvent<React.SyntheticEvent | DocumentEventMap[keyof DocumentEventMap] | {}> = {},
) => {
event.defaultMuiPrevented = false;
Copy link
Member

Choose a reason for hiding this comment

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

if we set event.defaultMuiPrevented = true; and then publish then it will be reset here and be published.
Why not check defaultMuiPrevented everywhere there isPropagationStopped

Copy link
Member Author

Choose a reason for hiding this comment

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

We still want the event to be published. Only the listeners should not run if event.defaultMuiPrevented is true.

@@ -12,7 +13,15 @@ export function useGridApiEventHandler(

React.useEffect(() => {
if (handler && eventName) {
return apiRef.current.subscribeEvent(eventName, handler, options);
const enhancedHandler = (
Copy link
Member

Choose a reason for hiding this comment

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

that could go out of this scope in a separate function. Not sure about redefining the handler as it changes the ref of the handler func potentially breaking immutability if (handler === otherHandler) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have any place where we compare the handler that is actually passed to subscribeEvent. I mean, compare the handler with the one that is saved in the event emitter. The ref would not change.

Another approach would be to move the event.defaultMuiPrevented validation to the event emitter, which is a very low level way to do it.

@m4theushw m4theushw merged commit 08dab61 into mui:master Jul 28, 2021
@m4theushw m4theushw deleted the prevent-default-behavior branch July 28, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Use event.defaultMuiPrevented to prevent default events
3 participants