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] Group events into a single enum #2279

Merged
merged 20 commits into from
Aug 10, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Aug 3, 2021

Breaking changes

-import { GRID_CELL_EDIT_START } from '@material-ui/x-grid';
-apiRef.current.subscribeEvent(GRID_CELL_EDIT_START, (params, event) => { ... });
+import { GridEvents } from '@material-ui/x-grid';
+apiRef.current.subscribeEvent(GridEvents.cellEditStart, (params, event) => { ... });

Part of #2227

With #2278 there is no change in the documentation output.

Several benefits here :

  • Improve the clarity of the public API by gathering dozens of string variables
  • Simplify the imports in our codebase
  • Help the future typing of publishEvent / subscribeEvent (they will be able to import the enum instead of having floating strings as keys

Do we want the user to use directly the string (like in https://github.com/mui-org/material-ui-x/blob/master/docs/src/pages/components/data-grid/events/SubscribeToEvents.tsx#L16) or do we want them to import the constant (like in https://github.com/mui-org/material-ui-x/blob/master/docs/src/pages/components/data-grid/editing/CatchEditingEventsGrid.tsx#L25)

If we want him to import the enum, could we type publishEvent / subscribeEvent first param accordingly ?

@flaviendelangle flaviendelangle self-assigned this Aug 3, 2021
@flaviendelangle flaviendelangle added the on hold There is a blocker, we need to wait label Aug 4, 2021
@flaviendelangle flaviendelangle removed the request for review from dtassone August 5, 2021 10:54
@flaviendelangle flaviendelangle marked this pull request as ready for review August 6, 2021 12:37
@flaviendelangle flaviendelangle added core Infrastructure work going on behind the scenes and removed on hold There is a blocker, we need to wait labels Aug 6, 2021
@flaviendelangle flaviendelangle added this to the Sprint 20 milestone Aug 6, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks like a great idea. I have merged HEAD into te PR to leverage #2278, showing the events.json has no diffs.

packages/grid/_modules_/grid/hooks/root/useEvents.ts Outdated Show resolved Hide resolved
packages/grid/_modules_/grid/constants/eventsConstants.ts Outdated Show resolved Hide resolved
@flaviendelangle flaviendelangle merged commit 9d89281 into mui:master Aug 10, 2021
@flaviendelangle flaviendelangle deleted the enum-events branch August 10, 2021 21:18
@oliviertassinari oliviertassinari added breaking change component: data grid This is the name of the generic UI component, not the React module! and removed core Infrastructure work going on behind the scenes labels Aug 12, 2021
@@ -112,7 +107,7 @@ export const useGridKeyboard = (apiRef: GridApiRef): void => {
}

if (isNavigationKey(event.key) && !isSpaceKey(event.key) && !event.shiftKey) {
apiRef.current.publishEvent(GRID_COLUMN_HEADER_NAVIGATION_KEY_DOWN, params, event);
apiRef.current.publishEvent(GridEvents.cellNavigationKeyDown, params, event);
Copy link
Member

Choose a reason for hiding this comment

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

It should have been:

Suggested change
apiRef.current.publishEvent(GridEvents.cellNavigationKeyDown, params, event);
apiRef.current.publishEvent(GridEvents.columnHeaderNavigationKeyDown, params, event);

The keyboard navigation is no longer working on the header https://codesandbox.io/s/material-demo-forked-git2x

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants