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] Add missing keys to the classes prop #2458

Merged
merged 9 commits into from
Sep 7, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Aug 26, 2021

Breaking changes

  • [DataGrid] Rename the .Mui-resizing CSS class to .MuiDataGrid-columnSeparator.

Preview: https://deploy-preview-2458--material-ui-x.netlify.app/api/data-grid/data-grid/#css

Closes #420

The docs generator uses the classes prop to extract information for the CSS section. Since we were supporting only a few keys, we would not be able to document all the classes available. This PR adds these missing keys. In components where multiple classes are applied depending on conditions, I implemented the useUtilityClasses pattern used in the core, but in most cases I went with clsx.

Part of this PR will conflict with #2433, specially where we replace "__" with "--". #2433 introduced a single hook to allow to pre-process the prop values, but here I duplicated the code between DataGrid and DataGridPro.

I renamed .MuiDataGrid-columnSeparator--dragging to .MuiDataGrid-columnHeader--dragging because in #2320 I used the wrong name: https://github.com/mui-org/material-ui-x/pull/2320/files#diff-0fae50bd4900beba39da8e79eedc2a3655445f14682f842c89b65b49e1a622dfL67

@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Aug 28, 2021
@flaviendelangle flaviendelangle self-requested a review September 2, 2021 07:26
@flaviendelangle
Copy link
Member

Seems good to me

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.

Looks like we are close, I have left a couple of questions

'checkboxInput',
'overlay',
'renderingZone',
'root',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep the classes in a DOM order rather than an alphabetical one. For instance, root is the first one people will see in the DOM. In the core, we use the DOM order, but I would understand that it might not work well when they are many classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

DOM order would make more sense. I'll reorder them in #2465. For now, the alphabetical order is fine.

@@ -105,7 +105,7 @@ const globalPseudoClassesMapping: Record<string, string> = {

export function generateUtilityClass(componentName: string, slot: string): string {
const globalPseudoClass = globalPseudoClassesMapping[slot];
return globalPseudoClass || `${componentName}-${slot}`;
return globalPseudoClass || `${componentName}-${slot}`.replace('__', '--');
Copy link
Member

Choose a reason for hiding this comment

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

I got a bit confused about the usage of __ for the JS classes API, and -- for the CSS class name API. Is the long-term plan to unify around __? I recall seeing an issue but I can't find it back. Maybe to open a new one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this part because I didn't receive many positive feedbacks about it in #420 (comment). We could re-evaluate if the BEM syntax is something we want to use in the future. I was trying to avoid having to use this syntax: https://github.com/mui-org/material-ui-x/pull/2458/files#diff-daa7ce589b380e02bf73ba2d9b08377cc7571039f62c5e057e027f3f0393779aR86

@m4theushw m4theushw merged commit 7f53f0c into mui:next Sep 7, 2021
@m4theushw m4theushw deleted the classes-prop branch September 7, 2021 21:26
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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Missing keys in classes prop
3 participants