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

Add <Datagrid rowSx> prop to customize row style for each record #8925

Merged
merged 8 commits into from
Aug 10, 2023

Conversation

zhujinxuan
Copy link
Contributor

@zhujinxuan zhujinxuan commented May 17, 2023

This PR intends to substitute rowStyle by rowSx for styling the Datagrid and SimpleList rows for two reasons:

  1. Sx is more common in mui instead of style to style.
  2. rowStyle will apply to the whole row. If one wants to highlight specific cells in the datagrid body, for example highlight duplicate phone numbers, they have to write their own DatagridBody and DatagridBodyRow. Meanwhile, rowSx can fix the problem.

Follow up of #8630

@zhujinxuan zhujinxuan changed the title [WIP] Replace rowStyle with rowSx Replace rowStyle with rowSx May 17, 2023
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks! This is nice but please make it an alternative to rowStyle, not a replacement.

@djhi djhi deleted the branch marmelab:next May 25, 2023 08:19
@djhi djhi closed this May 25, 2023
@djhi djhi reopened this May 25, 2023
@zhujinxuan
Copy link
Contributor Author

Sorry, I was busy this week. I will make the change next Tuesday.

@zhujinxuan zhujinxuan changed the title Replace rowStyle with rowSx [WIP] Replace rowStyle with rowSx May 25, 2023
@zhujinxuan zhujinxuan changed the title [WIP] Replace rowStyle with rowSx Replace rowStyle with rowSx Jun 13, 2023
@zhujinxuan
Copy link
Contributor Author

zhujinxuan commented Jun 13, 2023

Sorry for the delay, I have had a back pain in last few weeks that kept me away from computer. I have add rowStyle back to the document, to the ra-ui-materialui and to Datagrid.stories.tsx. But I keep the rowSx change in the demo and in the example because I think it would be better if the demo code is more consistent with the code style of mui.

Please let me know if there is any thing needed to be updated about this PR.

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks! I hope you feel better now :)

docs/Datagrid.md Outdated Show resolved Hide resolved
docs/SimpleList.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Great! Thanks!

@djhi djhi requested review from fzaninotto and slax57 June 14, 2023 08:12
@fzaninotto
Copy link
Member

Thanks for your PR!

However, I'm not very fond of this change.

First, it adds some duplication. I know it's for backward compatibility, but the doc for rowSx is almost the same as for rowStyle, and there is no indication of which prop to use, so I find it overall confusing.

Second, we already have an sx prop in Datagrid and SimpleList. If we want to offer something really better than rowStyle, why not introduce a rowClass prop instead, allowing to change the class name depending on some condition? That would allow to keep the datagrid styles in one place.

Waiting for more feedback to decide.

@zhujinxuan
Copy link
Contributor Author

zhujinxuan commented Jun 14, 2023

Hi, @fzaninotto . I made this change to have a method to style a single cell. We can add sx to Datagrid, but it cannot style single cell because cell classes in each row are the same. Currently, the only way to style a single cell is to use nth two times, which is not intuitive.

I think rowClass is also a solution (Indeed I think rowClass is better than rowSx). Shall we add a deprecation note of rowStyle after introducing rowClass?

BTW, if we decide to move on with rowClass, I can update this PR to a rowClass PR.

@djhi
Copy link
Collaborator

djhi commented Jun 16, 2023

I actually think rowSx to be pretty intuitive as you define the styles according to the record at the same place whereas rowClass would require you to check two different props to ensure you applied the correct className for the conditions you check on the record.

@slax57
Copy link
Contributor

slax57 commented Jul 3, 2023

Personally, I'm also in favor of adding the rowSx prop.
Besides, this feature allows for things like setting the style of the row directly depending on the record, for instance using a numeric value (like a percentage) from the record directly to compute a bg or fg color.
This would be way harder to do with rowClass.

@fzaninotto
Copy link
Member

Seems I'm in minority, so we'll go for it! I'll just deprecate the rowStyle property in another Pr to avoid confusion.

Thanks for your contribution!

@fzaninotto fzaninotto merged commit f0431ef into marmelab:next Aug 10, 2023
@fzaninotto fzaninotto added this to the 4.13.0 milestone Aug 10, 2023
@fzaninotto fzaninotto changed the title Replace rowStyle with rowSx Add <Datagrid rowSx> prop to customize row style for each record Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants