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] Fix events naming #1862

Merged
merged 8 commits into from
Jun 21, 2021
Merged

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Jun 9, 2021

Breaking changes

  • [XGrid] The following events were renamed:

    1. columnHeaderNavigationKeydown to columnHeaderNavigationKeyDown
    2. columnResizeCommitted to columnWidthChange
    3. rowsUpdated to rowsUpdate
    4. rowsCleared to rowsClear
    5. columnsUpdated to columnsChange
  • [XGrid] The following DOM events were removed because they were not necessary for the grid to work:

    1. focusout
    2. keydown
    3. keyup

    If you need them, attach a listener using the apiRef, as following:

    apiRef.current.rootElementRef?.current?.addEventListener('focusout', event => {
      // ...
    });

Preview: https://deploy-preview-1862--material-ui-x.netlify.app/components/data-grid/events/#catalog-of-events
Closes #1383

@m4theushw m4theushw added breaking change component: data grid This is the name of the generic UI component, not the React module! labels Jun 9, 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.

I'm confused. Why do we have GRID_COLUMN_HEADER_KEYDOWN but GRID_COLUMN_HEADER_NAVIGATION_KEY_DOWN (KEY_DOWN vs. KEYDOWN)?

@@ -417,7 +390,7 @@ export const GRID_COLUMN_ORDER_CHANGE = 'columnOrderChange';
* @ignore - do not document.
* @event
*/
export const GRID_ROWS_UPDATED = 'rowsUpdated';
export const GRID_ROWS_UPDATE = 'rowsUpdate';
Copy link
Member

Choose a reason for hiding this comment

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

GRID_ROWS_CHANGE then?
as you remove 'update' in columns

Copy link
Member

@oliviertassinari oliviertassinari Jun 10, 2021

Choose a reason for hiding this comment

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

@dtassone and how would you name the two other events? GRID_ROWS_SET, GRID_ROWS_CLEAR.

If we update the row update events to only fire one, then GRID_ROWS_CHANGE seem definitely better.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the point to have GRID_ROWS_CLEAR? It's only used by the sorting to clear the sorting once all the records are updated. GRID_ROWS_SET could be used instead.

Copy link
Member

Choose a reason for hiding this comment

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

Well I like granular events and those are for internal use but ultimately they are change events.
However the clear and set occurs in 1 rendering so you can't publish change as it's for internal reset of rows to put new ones. Users should only know about the new updated set.

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 have doubts, how about we keep those for the next iteration?

Copy link
Member

@oliviertassinari oliviertassinari Jun 18, 2021

Choose a reason for hiding this comment

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

Looking at the publish:

  • When GRID_ROWS_CLEARED is triggered, GRID_ROWS_SET is always triggered.

At the listeners:

  • GRID_ROWS_SET is listed by applySorting, applySorting
  • GRID_ROWS_CLEARED is listened by "empty sorting" (I'm sending a PR to remove it, it's noise: [DataGrid] Make GRID_ROWS_CLEAR private #1925)
  • GRID_ROWS_UPDATED is listened by applySorting, applyFilters

So it seems that we only need one event, not three.

Copy link
Member

Choose a reason for hiding this comment

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

GRID_ROWS_CLEARED allows to clear the sortedRows state.

Copy link
Member

Choose a reason for hiding this comment

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

the split between GRID_ROWS_UPDATED and GRID_ROWS_SET was more for a user perspective where one would want to subscribe to row updates

Copy link
Member Author

@m4theushw m4theushw Jun 18, 2021

Choose a reason for hiding this comment

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

GRID_ROWS_SET is published by setRows. But this method is mostly used in demos that programmatically call the API, so the user calling the method doesn't need an event and we can remove it. The only use of setRows outside demos is in updateRows, which also publishes GRID_ROWS_UPDATED.

Side note: setRows is actually appending rows, renaming to addRows might be better.

Copy link
Member

Choose a reason for hiding this comment

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

Originally the idea was to do a reset with a clear and a set, to avoid sortedRows error... If we remove the clear, we can remove the set as well and just rename the updated to change.

@oliviertassinari

This comment has been minimized.

@m4theushw m4theushw requested review from DanailH and removed request for dtassone June 15, 2021 14:46
@m4theushw m4theushw merged commit 1210f1a into mui:master Jun 21, 2021
@m4theushw m4theushw deleted the fix-events-naming branch June 21, 2021 19:42
oliviertassinari pushed a commit to oliviertassinari/mui-x that referenced this pull request Jun 21, 2021
oliviertassinari pushed a commit to oliviertassinari/mui-x that referenced this pull request Jun 21, 2021
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.

[RFC] Event naming conventions
3 participants