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] Rename components and componentsProps props #7059

Closed

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Dec 1, 2022

Fixes #6986

Breaking change

-<DataGrid components={} componentsProps={}>
+<DataGrid slots={} slotsProps={}>

PS: @flaviendelangle it probably makes sense to do the same change for the pickers

@DanailH DanailH changed the title Rename components and componentsProps props [DataGrid] Rename components and componentsProps props Dec 1, 2022
@DanailH DanailH self-assigned this Dec 1, 2022
@DanailH DanailH added breaking change component: data grid This is the name of the generic UI component, not the React module! labels Dec 1, 2022
@flaviendelangle
Copy link
Member

@DanailH With @joserodolfofreitas , we discussed keeping both names during v6.
Our goal was to discuss it during the monthly meeting to see how the core was approaching the topic, to avoid having big inconsistencies across all the packages.
I don't know what we will end up doing, but we should definitely discuss it and decide before merging this PR.

@alexfauquette will do the implementation on the picker side.

@mui-bot
Copy link

mui-bot commented Dec 1, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-7059--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 604.5 820.7 687.6 709.9 94.113
Sort 100k rows ms 595.7 996.1 811.8 811.12 132.884
Select 100k rows ms 198.6 292 210.3 235.6 40.909
Deselect 100k rows ms 137.1 203 176.5 174.46 24.894

Generated by 🚫 dangerJS against d3f37d5

@cherniavskii
Copy link
Member

We have briefly discussed it last week and it sounded like we were leaning toward the same approach as Material UI (option 2 in #6986).
But there was no decision on that yet.
cc @joserodolfofreitas

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Dec 1, 2022

Perhaps it's a better idea to try closing this topic earlier at the General meeting tomorrow, we'll have more time to discuss the trade-offs.

From DX side, I think it's positive to keep both APIs (during v6) and provide consistency for users using Material UI. It's also generally a more friendly approach, which is particularly important when updating one of the core concepts for customization. The negative aspect I see is the possibility of users relying too much on copying and pasting and ending up with both the slots and components set, which in this case, they should be warned that only slots property will be considered (however, that's also a realistic scenario if we make the breaking change now).

From the development side, as I understand, we'd have to maintain (during v6) a few duplicated APIs and types. I guess most of the deprecation warnings and error treatment would be the same, probably with some additional logic conditional statements.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2022
@github-actions
Copy link

github-actions bot commented Dec 2, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@@ -13,7 +13,7 @@ import {
import { useBasicDemoData } from '@mui/x-data-grid-generator';
import { getCell, getRow } from 'test/utils/helperFn';

describe('<DataGridPro/> - Components', () => {
describe('<DataGridPro/> - slots', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('<DataGridPro/> - slots', () => {
describe('<DataGridPro/> - Slots', () => {

@@ -11,7 +11,7 @@ import { spy } from 'sinon';
import { DataGrid, GridOverlay } from '@mui/x-data-grid';
import { getCell, getRow } from 'test/utils/helperFn';

describe('<DataGrid /> - Components', () => {
describe('<DataGrid /> - slots', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('<DataGrid /> - slots', () => {
describe('<DataGrid /> - Slots', () => {

const components = React.useMemo<GridSlotsComponent>(() => {
const overrides = themedProps.components;
const slots = React.useMemo<GridSlotsComponent>(() => {
const overrides = themedProps.slots;
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to continue extending from components while MUI Core has not renamed the prop on their side.

Suggested change
const overrides = themedProps.slots;
const overrides = themedProps.components;

@@ -86,7 +86,7 @@ export type DataGridForcedPropsKey =
* The `DataGrid` options with a default value that must be merged with the value given through props.
*/
export interface DataGridPropsWithComplexDefaultValueAfterProcessing {
components: GridSlotsComponent;
slots: GridSlotsComponent;
Copy link
Member

Choose a reason for hiding this comment

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

We need to decide which approach to follow:

  1. Keep only slots
  2. Only support slots and show a warning if components is used
  3. Recommend slots but continue supporting components with a warning

cc @mui/x

Copy link
Member

Choose a reason for hiding this comment

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

I'd say go for no. 3 for v6. It'll ease down some of the migration pain. Probably remove completely in v7?

Copy link
Member

Choose a reason for hiding this comment

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

supporting components with a warning

Maybe mark those props as @deprecated instead of a warning in the console?

Copy link
Member

Choose a reason for hiding this comment

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

@MBilalShafi
Copy link
Member

Should we additionally follow these conventions too? (From mui/material-ui#34693)

  1. componentsProps -> slotsProps
  2. change of casing in slots' fields from Pascal-case to camelCase (Root -> root)

I think pickers followed it (at least the slotProps part)

@MBilalShafi
Copy link
Member

Has been handled in #7882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] components -> slots API rename
7 participants