[docs] Add description for all events #1572

m4theushw · 2021-05-06T00:17:09Z

Breaking changes

[XGrid] Event scrolling:rows renamed to rowsScroll for consistency.
[XGrid] Event scroll:rowEnd renamed to rowsScrollEnd for consistency.

m4theushw · 2021-05-06T02:30:52Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+
+/**
+ * @ignore - do not document.
+ */
 export const GRID_ELEMENT_FOCUS_OUT = 'gridFocusOut';


I ignored it because I think this event could be removed. It's only used to detect if the multiple key is pressed and we agreed that the event argument should be used a single source of truth. Also, its name is confusing and hard to explain the conditions in which the event is fired.

packages/grid/_modules_/grid/constants/eventsConstants.ts

m4theushw · 2021-05-06T02:35:01Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+
+/**
+ * @ignore - do not document.
+ */
 export const GRID_NATIVE_SCROLL = 'scroll';


It's never published.

Dead code to remove then?

The constant is only used here: https://github.com/mui-org/material-ui-x/blob/master/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualRows.ts#L411. Maybe it should be a local variable.

m4theushw · 2021-05-06T02:42:24Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+/**
+ * Fired when a `mousedown` event happens in a cell. Called with a [[GridCellParams]] object.
+ * @event
+ */
 export const GRID_CELL_MOUSE_DOWN = 'cellMouseDown';


Should we have this level of granularity when exposing events? I think we should document only the most obvious events (click, double-click, keydown) and wait users to request more.

IMO it is part of the api so ppl should be free to use it. I don't see the issue.

Documenting only the most obvious is not a proper api documentation as there will be missing things so the doc will be incomplete as soon as it's updated. Ppl will have to look at the typings to get the most accurate doc.

The issue is about the long tail of use cases. What about touch start, touch move, touch end, or even the pointers events https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#event_types_and_global_event_handlers? It doesn't really scale.

I think we should think about what users would potentially create with these events. We are exposing all the column header drag events, but I don't think users would write another drag-and-drop feature from scratch. Having all the events we use internally makes the documentation long and may force users to question themselves: which event should I subscribe? mousedown or click?

or force us to hold on refactoring until the next breaking changes release window (developers hate BCs, and enjoy handling them in a batch).

I marked more events with @ignore. My rationale is:

Ignore drag-and-drop events because they have a very specific use.

Ignore the less obvious events published by column headers (keep only click, double and keydown).

Ignore global events (GRID_FOCUS_OUT) because users can already subscribe to them using the ref prop. Also, if we're going to document them we should document EVERYTHING.

This is from ag grid https://www.ag-grid.com/react-grid/grid-events/

rowDataChanged The client has set new data into the grid using api.setRowData() or by changing the rowData bound property.

rowDataUpdated The client has updated data for the grid using api.applyTransaction(transaction) or by changing the rowData bound property with immutableData=true.

rowDragEnter A drag has started, or dragging was already started and the mouse has re-entered the grid having previously left the grid.

rowDragMove The mouse has moved while dragging.

rowDragLeave The mouse has left the grid while dragging.

rowDragEnd The drag has finished over the grid.

RowEvent
├── RowSelectedEvent {}
├── RowClickedEvent {}
├── RowDoubleClickedEvent {}
├── RowEditingStartedEvent {}
├── RowEditingStoppedEvent {}
├── RowGroupOpenedEvent {
├── RowValueChangedEvent {}
├── VirtualRowRemovedEvent {}
└── CellEvent
├── CellClickedEvent {}
├── CellMouseDownEvent {}
├── CellDoubleClickedEvent {}
├── CellMouseOverEvent {}
├── CellMouseOutEvent {}
├── CellContextMenuEvent {}
├── CellEditingStartedEvent {}
├── CellKeyDown {}
├── CellKeyPress {}
├── CellEditingStoppedEvent
└── CellValueChangedEvent

m4theushw · 2021-05-06T02:43:45Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+/**
+ * Fired when the rows are updated.
+ * @event
+ */
 export const GRID_ROWS_SET = 'rowsSet';


I couldn't understand the difference between this one and GRID_ROWS_UPDATED.

GRID_ROWS_SET is published when the full set of rows is reset.
GRID_ROWS_UPDATED is published when you update one or more rows.

I think that the underlying question is: why does the difference matter (from a developer problem perspective)? For instance, the data grid handles the two events the same way. What if we merged them?

No it should not be the same. One reset the prop and do a full rerender and the other one is more optimised

@dtassone Can you describe the difference in a short text to add in the description?

it's also when rows length change

Oh so if a developer uses the imperative API to add or remove records, then GRID_ROWS_SET is fired. I thought it was only for the rows prop. (One example of confusion in the description 😁). Would this be clearer? (docs is a lot about avoiding confusion)

GRID_ROWS_SET: Fired when the length of the rows changes or when a new set of rows is provided.

GRID_ROWS_UPDATED: Fired when at least one row is updated in place.

What happen for row deletions, is only GRID_ROWS_SET fired?

I would not document both GRID_ROWS_UPDATED and GRID_ROWS_SET. It's not clear the conditions in which they are fired.

How will a developer understand what "reset" mean? Should we mention that it triggers when the rows prop change?

It's not triggered when the rows prop change, but when calling apiRef.current.updateRows. This should be clear in the description.

Reset word is pretty common, in the computer world...

IHMO this "reset" word may confuse users. One of the definitions is "to set again". But in the React world "set again" might not cause a rerender if the value is the same. If we are using "reset" in the context of when the value of something changes, this should be more explicit, maybe using "change".

Feel free to call it GRID_ROWS_CHANGE I have no strong opinion anymore.

Reset as it does a clear and set to update the sort, filters...

GRID_ROWS_SET: Fired when the length of the rows changes or when a new set of rows is provided.
GRID_ROWS_UPDATED: Fired when at least one row is updated in place.

👍

I don't see why we are restricting, if some advanced trading app needs to use those events, let them do, that's why the grid is based on an event emitter, so it could be hacked, customized...
We can reassess at a later stage, this code will be refactored along with time, things will evolve over the versions.

We can reassess at a later stage, this code will be refactored along with time, things will evolve over the versions.

That's a reason I would not document these two events. Once we have clearly conditions of when they should be published we can add to the docs. Users are free to use them anytime, it's just not exposed in the page.

AG-Grid has an event rowDataChanged that is published both when the prop changes and when programmatically changing the data.

#1572 (comment)

m4theushw · 2021-05-06T02:49:26Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+ * Fired after the filter model is changed and the new model is applied.
+ * Called with a [[GridFilterModelParams]] object.
+ * @event
+ */
 export const GRID_FILTER_MODEL_CHANGE = 'filterModelChange';


Side note: the description of the onFilterModelChange prop is wrong. This event is published after applying the filter model, not before.

It looks like the implementation is wrong then. As far as I know, the application of the new model is a different concern it should always come after.

Regarding "Fired after the filter model is changed", IMHO "after" doesn't matter, it's about the user event action.

Description updated.

packages/grid/_modules_/grid/constants/eventsConstants.ts

oliviertassinari · 2021-05-06T11:10:40Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+/**
+ * Fired when a `mousedown` event happens in a cell. Called with a [[GridCellParams]] object.
+ * @event
+ */
 export const GRID_CELL_MOUSE_DOWN = 'cellMouseDown';


The issue is about the long tail of use cases. What about touch start, touch move, touch end, or even the pointers events https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events#event_types_and_global_event_handlers? It doesn't really scale.

oliviertassinari · 2021-05-06T11:12:51Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+/**
+ * Fired when the rows are updated.
+ * @event
+ */
 export const GRID_ROWS_SET = 'rowsSet';


I think that the underlying question is: why does the difference matter (from a developer problem perspective)? For instance, the data grid handles the two events the same way. What if we merged them?

oliviertassinari · 2021-05-06T11:16:32Z

packages/grid/_modules_/grid/constants/eventsConstants.ts

+ * Fired after the filter model is changed and the new model is applied.
+ * Called with a [[GridFilterModelParams]] object.
+ * @event
+ */
 export const GRID_FILTER_MODEL_CHANGE = 'filterModelChange';


It looks like the implementation is wrong then. As far as I know, the application of the new model is a different concern it should always come after.

Regarding "Fired after the filter model is changed", IMHO "after" doesn't matter, it's about the user event action.

packages/grid/_modules_/grid/constants/eventsConstants.ts

m4theushw added 3 commits May 5, 2021 21:12

add description for all events

79821b7

update constant value

bc9834a

typos

ed14d86

m4theushw mentioned this pull request May 6, 2021

[docs] Automatically generate API docs #1529

Merged

7 tasks

m4theushw commented May 6, 2021

View reviewed changes

oliviertassinari reviewed May 6, 2021

View reviewed changes

packages/grid/_modules_/grid/constants/eventsConstants.ts Show resolved Hide resolved

oliviertassinari reviewed May 6, 2021

View reviewed changes

oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels May 6, 2021

update events

27bf2a0

m4theushw mentioned this pull request May 7, 2021

[DataGrid] Add onColumnVisibilityChange prop #1578

Merged

m4theushw added 2 commits May 10, 2021 15:01

Merge branch 'master' into event-descriptions

6255f45

Merge branch 'master' into event-descriptions

483dadf

dtassone approved these changes May 17, 2021

View reviewed changes

m4theushw merged commit b733055 into mui:master May 17, 2021

m4theushw deleted the event-descriptions branch May 17, 2021 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Add description for all events #1572

[docs] Add description for all events #1572

m4theushw commented May 6, 2021 •

edited

Loading

m4theushw May 6, 2021

m4theushw May 6, 2021

oliviertassinari May 6, 2021

m4theushw May 6, 2021

m4theushw May 6, 2021

dtassone May 6, 2021

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021

dtassone May 7, 2021

m4theushw May 6, 2021

dtassone May 6, 2021

oliviertassinari May 6, 2021

dtassone May 6, 2021

m4theushw May 6, 2021

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021 •

edited

Loading

dtassone May 6, 2021

m4theushw May 6, 2021

dtassone May 7, 2021

m4theushw May 6, 2021

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021

oliviertassinari May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021

oliviertassinari May 6, 2021 •

edited

Loading

rowDragEnter	A drag has started, or dragging was already started and the mouse has re-entered the grid having previously left the grid.
rowDragMove	The mouse has moved while dragging.
rowDragLeave	The mouse has left the grid while dragging.
rowDragEnd	The drag has finished over the grid.

[docs] Add description for all events #1572

[docs] Add description for all events #1572

Conversation

m4theushw commented May 6, 2021 • edited Loading

Breaking changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

m4theushw May 6, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

m4theushw May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 6, 2021 • edited Loading

Choose a reason for hiding this comment

m4theushw commented May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

m4theushw May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading

oliviertassinari May 6, 2021 •

edited

Loading