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 columnHeader, row and cell in addition to root in classes prop #1660

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented May 13, 2021

Fixes #1495

Not much in terms of the approach. I had to add the classes to the grid options in order to have access to them in the column header, row, and cell components.

BREAKING CHANGES:

-.MuiDataGrid-colCellWrapper
-.MuiDataGrid-colCell
+.MuiDataGrid-columnHeaderWrapper
+.MuiDataGrid-columnHeader
-.MuiDataGrid-colCellCheckbox
+.MuiDataGrid-columnHeaderCheckbox
-.MuiDataGrid-colCellSortable
+.MuiDataGrid-columnHeaderSortable
-.MuiDataGrid-colCellCenter
-.MuiDataGrid-colCellRight
-.MuiDataGrid-colCellTitle
+.MuiDataGrid-columnHeaderCenter
+.MuiDataGrid-columnHeaderRight
+.MuiDataGrid-columnHeaderTitle

Missing:

  • Update /style page in the docs

https://deploy-preview-1660--material-ui-x.netlify.app/components/data-grid/style/#using-the-classes-object-prop

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels May 13, 2021
@DanailH DanailH self-assigned this May 13, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Added columnHeader, row and cell in addition to root in classes prop [DataGrid] Add columnHeader, row and cell in addition to root in classes prop May 14, 2021
docs/src/pages/components/data-grid/style/style.md Outdated Show resolved Hide resolved
@@ -98,6 +98,14 @@ const columns: GridColumns = [

{{"demo": "pages/components/data-grid/style/StylingCellsGrid.js", "bg": "inline"}}

## Using the `classes` object prop
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we should document the classes. This is most often a foot-gun, here is an example why:

Screenshot 2021-05-14 at 15 00 51

Or maybe we should only add a strong warning about it

Copy link
Member

Choose a reason for hiding this comment

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

Well what's the point of doing the feature if we don't want to show it to the users?

Copy link
Member

Choose a reason for hiding this comment

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

The main value I think is about having the same API as in the core components. It's only a foot gun because we increase specificity for performance. This mean that developers will lose specificity over the default styles of the grid.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a warning about this, to prefix the demo would be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

When doing the demo it felt like a duplicate. I can remove the demo and just add a link to the page that explains how to use classes?

@@ -82,3 +84,7 @@ export function mapColDefTypeToInputType(type: string) {

// Util to make specific interface properties optional
export type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>;

export function getButtonBaseUtilityClass(slot) {
return generateUtilityClass(GRID_CSS_CLASS_PREFIX, slot);
Copy link
Member

Choose a reason for hiding this comment

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

what is this one? What issue does it solve?

Copy link
Member

Choose a reason for hiding this comment

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

This one is used to generate the class names. We didn't complete the migration, we are not supposed to have GRID_HEADER_CELL_CSS_CLASS variables like with the approach of v5 in the core. Instead, we would read the global gridClasses.headerCell.

@@ -98,6 +98,14 @@ const columns: GridColumns = [

{{"demo": "pages/components/data-grid/style/StylingCellsGrid.js", "bg": "inline"}}

## Using the `classes` object prop
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a warning about this, to prefix the demo would be enough?

packages/grid/_modules_/grid/components/GridViewport.tsx Outdated Show resolved Hide resolved
packages/grid/_modules_/grid/utils/utils.ts Outdated Show resolved Hide resolved
@@ -82,3 +84,7 @@ export function mapColDefTypeToInputType(type: string) {

// Util to make specific interface properties optional
export type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>;

export function getButtonBaseUtilityClass(slot) {
return generateUtilityClass(GRID_CSS_CLASS_PREFIX, slot);
Copy link
Member

Choose a reason for hiding this comment

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

This one is used to generate the class names. We didn't complete the migration, we are not supposed to have GRID_HEADER_CELL_CSS_CLASS variables like with the approach of v5 in the core. Instead, we would read the global gridClasses.headerCell.

@DanailH
Copy link
Member Author

DanailH commented May 17, 2021

Ok, I've renamed the classes from colCell to columnHeader for consistency and removing the use of abbreviation. This is now a breaking change PR, I'll list what those changes are in the PR description.

@DanailH DanailH merged commit 6878633 into mui:master May 17, 2021
Comment on lines +8 to +12
export const GRID_COLUMN_HEADER_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_COLUMN_HEADER_CSS_CLASS_SUFFIX}`;
export const GRID_ROW_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_ROW_CSS_CLASS_SUFFIX}`;
export const GRID_CELL_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_CELL_CSS_CLASS_SUFFIX}`;
export const GRID_COLUMN_HEADER_SEPARATOR_RESIZABLE_CSS_CLASS = `${GRID_ROOT_CSS_CLASS_SUFFIX}-columnSeparatorResizable`;
export const GRID_COLUMN_HEADER_TITLE_CSS_CLASS = `${GRID_ROOT_CSS_CLASS_SUFFIX}-columnHeaderTitleContainer`;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we will need to move the logic into a plain dataGridClasses file, as in the core.

Suggested change
export const GRID_COLUMN_HEADER_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_COLUMN_HEADER_CSS_CLASS_SUFFIX}`;
export const GRID_ROW_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_ROW_CSS_CLASS_SUFFIX}`;
export const GRID_CELL_CSS_CLASS = `${GRID_CSS_CLASS_PREFIX}-${GRID_CELL_CSS_CLASS_SUFFIX}`;
export const GRID_COLUMN_HEADER_SEPARATOR_RESIZABLE_CSS_CLASS = `${GRID_ROOT_CSS_CLASS_SUFFIX}-columnSeparatorResizable`;
export const GRID_COLUMN_HEADER_TITLE_CSS_CLASS = `${GRID_ROOT_CSS_CLASS_SUFFIX}-columnHeaderTitleContainer`;

@oliviertassinari
Copy link
Member

oliviertassinari commented May 18, 2021

Related to #408

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Create classes input to style rows, cells, and column headers
4 participants