[DataGrid] Refactor useGridColumnReorder

[m4theushw]

Breaking changes

• [DataGrid] Renamed all events related to column reordering, e.g. GRID_COL_REORDER_START -> GRID_COLUMN_REORDER_START
• [DataGrid] Methods onColItemDragStart, onColHeaderDragOver, onColItemDragOver, onColItemDragEnter removed from the grid API. Prefer listening to column reordering events
• [DataGrid] Call to apiRef.current.getColumnHeaderParams returns a GridColumnHeaderParams instead of GridColParams
• [DataGrid] Events GRID_COLUMN_HEADER_ will be called with a GridColumnHeaderParams instead of GridColParams
• [DataGrid] The renderHeader method will be called with a GridColumnHeaderParams instead of GridColParams
• [DataGrid] The apiRef.current.moveColumn API was renamed to apiRef.current.setColumnIndex

---

Closes #1201.

[dtassone]

You need to publish the event with params. For cell, we have GridCellParams, could you add GridColumnHeaderParams please

[oliviertassinari]

@m4theushw If you could update the issue's description to include all the breaking changes so we can include them in the next release changelog, it would be perfect.

[dtassone]

FYI more about events in #1301

[dtassone]

We should discuss how to present events in the doc.
This is the constant name not its value. Would users use the constant or its value?
I think we should show both, in a separate event page that we would link maybe at the bottom of this block

[oliviertassinari]

+1 for:

• making all the event const private
• only using the string values
• improve the types to only accept valid event strings

This way, we get the benefit of:

• Only one way to do things (we have to choose anyway what we encourage in the demos)
• No pain of importing variables to subscribe to events (e.g. document.addEventerListener('resize'))
• Retain type safety

[m4theushw]

DataTables and AG Grid only present the events as strings in their documentations.

Firebase present the const and the value.

Reasons in favor of exporting event consts:

• Avoid hardcoded strings
• Refactoring is easier

I would go with not exporting consts for now, since they are changing. In the future, they could be exported for those users who prefer to use a const instead of strings.

[oliviertassinari]

I guess I don't get how doing

import { GRID_COLUMN_REORDER_START } from '@material-ui/x-grid';

apiRef.current.on(GRID_COLUMN_REORDER_START, () => {});

is better than:

apiRef.current.on('gridColumnReoederStart', () => {});

It seems to yield the same features, without the import pain:

• if you did a typo, type safety get you covered
• if we release a breaking change, type safety get you covered
• if you want to refactor, the string is long enough to run search and replace

I personally think that the const is only interesting for JavaScript users, that don't get type errors.

---

But I think that what's most important is the documentation. What do we use? For the demos, we can only use one variant. For the markdown, we could potentially describe the two but it would create confusion among developers (which one should I use ?!). So I think that we should first answer the following:

Assuming we only document one variant. Which one do we use in the documentation?

How about we only use the const at the cost of making it harder to use, but at the pros of making it better for JavaScript users? Developers that prefer string, can log the content of the constant, and we can explain to them how the camelCase convention work.

[m4theushw]

Assuming we only document one variant. Which one do we use in the documentation?

+1 for eventName in the demos and markdown pages. The only exception to show both would be in the API page. There could have a "Events" section with all strings and their equivalent constants, potentially showing the arguments like Kendo UI does. BTW they use save and SaveEvent in the demos because of the way Angular binds events.

I think something we're missing here is a stable naming convention for the events. Having columnReordering:dragStart and columnHeaderClick may create some confusion.

[oliviertassinari]

@m4theushw eventName over EVENT_NAME?

Should we do a vote? :)

• 🎉 eventName
• 🚀 EVENT_NAME

[dtassone]

small one. Could we type the params before publishing. So if we change the params then it breaks

[dtassone]

Also where is options.onColumnOrderChange hooked to the event?

[dtassone]

useGridApiOptionHandler(apiRef, GRID_COLUMN_ORDER_CHANGE, options.onColumnOrderChange) ?

[m4theushw]

Also where is options.onColumnOrderChange hooked to the event?

Do we need to call it with the event? Users can get it by subscribing to GRID_COLUMN_REORDER_DRAG_END.

[oliviertassinari]

A note for the future self (outside of the scope of this PR) and Matheus, React.FC is wrong, the component has no children prop

[m4theushw]

True, another PR to update all components that don't use children would be good.

[oliviertassinari]

@m4theushw Do you want to open a new issue for keeping track of it? I'm not saying it's a priority but, that it would be great to handle it, at some point. Some of these components are public, hence hurt the developers using them.

[m4theushw]

#1325

[dtassone]

Nice one 👍