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] Remove apiRef from options params #2312

Merged

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Aug 9, 2021

Fixes #1821
Fixes #1301

This is part 2 of 2 for #1821. As we discussed during the call the api is removed from all options callbacks. It is only present in what callbacks are in the GridColDef.


Breaking changes

All DataGrid props callbacks don't provide access to api anymore. To access api you need to use XGrid and access it from the details parameter.

<XGrid 
  onColumnResize={(params, event, details) => console.log(details.api)}
/>
  • GridStateChangeParams is removed, instead onStateChange first param is GridState
- onStateChange?: (params: GridStateChangeParams, event: MuiEvent<{}>, details: any) => void; 
+ onStateChange?: (state: GridState, event: MuiEvent<{}>, details: any) => void; 
export interface GridColumnHeaderParams {
   * The column of the current header component.
   */
  colDef: GridStateColDef;
-  /**
-  * API ref that let you manipulate the grid.
-  */
- api: any;
}
export interface GridColumnOrderChangeParams {
   * The old column index.
   */
  oldIndex: number;
-  /**
-   * API ref that let you manipulate the grid.
-   */
-  api: any;
}
export interface GridColumnResizeParams {
   * The column of the current header component.
   */
  colDef: GridStateColDef;
-  /**
-   * API ref that let you manipulate the grid.
-   */
-  api: any;
export interface GridColumnVisibilityChangeParams {
   * The column of the current header component.
   */
  colDef: GridStateColDef;
-  /**
-   * API ref that let you manipulate the grid.
-   */
-  api: any;
export interface GridRowParams {
   * All grid columns.
   */
  columns: GridColumns;
-  /**
-   * GridApiRef that let you manipulate the grid.
-   */
-  api: any;
export interface GridRowScrollEndParams {
   * The grid visible columns.
   */
  visibleColumns: GridColumns;
-  /**
-   * API ref that let you manipulate the grid.
-   */
-  api: any;

@DanailH DanailH added breaking change component: data grid This is the name of the generic UI component, not the React module! labels Aug 9, 2021
@DanailH DanailH self-assigned this Aug 9, 2021
@DanailH DanailH added this to the Sprint 20 milestone Aug 10, 2021
@DanailH DanailH changed the title Bug/data grid 1821 remove api ref from options params [DataGrid] Remove api ref from options params Aug 10, 2021
@DanailH DanailH changed the title [DataGrid] Remove api ref from options params [DataGrid] Remove apiRef from options params Aug 10, 2021
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

It seems weird to me to remove api from GridRowParams but keep it in GridCellParams. I would remove from both and for those places where it needs to be passed we add a child interface including it: e.g. ValueParserParams.

I found GridCellOptionsParams a bit confusing, because options and params are the same thing. Maybe by doing the first proposal we don't need it.

DanailH and others added 2 commits August 11, 2021 16:10
…ws.ts

Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@DanailH
Copy link
Member Author

DanailH commented Aug 11, 2021

It seems weird to me to remove api from GridRowParams but keep it in GridCellParams. I would remove from both and for those places where it needs to be passed we add a child interface including it: e.g. ValueParserParams.

I found GridCellOptionsParams a bit confusing, because options and params are the same thing. Maybe by doing the first proposal we don't need it.

Ok, the problem with that aproach is that by keeping the GridCellParams without api in it and changing the types to all ColDef callback to ValueParserParams which includes the api the typing won't be descriptive. Basically, the renderCell will get params of type ValueParserParams?

I would keep it as it is but maybe change the name of the interface GridCellOptionsParams. I agree there are better names for it so I'm open to suggestions.

@m4theushw
Copy link
Member

m4theushw commented Aug 11, 2021

Basically, the renderCell will get params of type ValueParserParams?

No, we would create dedicated interfaces for each of these callbacks. In the case of renderCell it could be RenderCellParams. We're already doing that for some callbacks:

https://github.com/mui-org/material-ui-x/blob/8deeab3b2400f0a91b90f9cea4b2a82fe2d8f6d8/packages/grid/_modules_/grid/models/colDef/gridColDef.ts#L91

https://github.com/mui-org/material-ui-x/blob/8deeab3b2400f0a91b90f9cea4b2a82fe2d8f6d8/packages/grid/_modules_/grid/models/colDef/gridColDef.ts#L97

The reason I'm against keeping the api is that it's not consistent and we need to remember to always delete it when passing the params as argument.

@DanailH
Copy link
Member Author

DanailH commented Aug 11, 2021

But you would still need to pass in the api for those callbacks.

@m4theushw
Copy link
Member

m4theushw commented Aug 11, 2021

The api would still be passed but it would be in a dedicated interface only containing the params related to cell rendering.

This component could use RenderCellParams instead of GridCellParams.

https://github.com/mui-org/material-ui-x/blob/8deeab3b2400f0a91b90f9cea4b2a82fe2d8f6d8/packages/grid/_modules_/grid/components/cell/GridBooleanCell.tsx#L5

One of our technical debts is that we used GridCellParams a lot only to pass the id and the field and to remove it now it will be a pain.

@m4theushw m4theushw mentioned this pull request Aug 12, 2021
@oliviertassinari oliviertassinari modified the milestones: Sprint 20, Sprint 21 Aug 16, 2021
@DanailH
Copy link
Member Author

DanailH commented Aug 16, 2021

@m4theushw @oliviertassinari I've updated the GridCellParams so now it doesn't contain api anymore and api is explicitly added to the GridColDef methods that need it

@oliviertassinari
Copy link
Member

I have resolved the conflict that I introduced with #2340 (I work on this after seeing the same in this PR).

@DanailH DanailH requested a review from m4theushw August 17, 2021 08:25
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Nice work!

@DanailH DanailH merged commit 0867157 into mui:master Aug 17, 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.

[DataGrid] Should not expose apiRef [DataGrid] Standardize/Reconcile all event handlers and params
4 participants