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] Update column menu API components -> slots #7999

Merged
merged 4 commits into from
Mar 27, 2023

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Feb 21, 2023

Final part of #6986
In continuity of #7882

Updated docs: https://deploy-preview-7999--material-ui-x.netlify.app/x/react-data-grid/column-menu/

Approach Followed:

Simple replace components and other references to slots. Didn't deprecate or list in breaking changes as the API was initially released in alpha.

Column menu components ->
slots
@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! v6.x labels Feb 21, 2023
@mui-bot
Copy link

mui-bot commented Feb 21, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-7999--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 620.8 1,011.9 620.8 780.32 187.888
Sort 100k rows ms 606 1,097 884.2 874.94 156.493
Select 100k rows ms 257.7 324.8 301.2 298.6 22.449
Deselect 100k rows ms 177 270 193.4 204.5 33.911

Generated by 🚫 dangerJS against 2d57c02

@MBilalShafi MBilalShafi marked this pull request as ready for review February 21, 2023 11:26
@flaviendelangle
Copy link
Member

Was the decision to entirely drop the components prop discussed during a meeting ?

For me the main argument in favor of keeping both was to let people use the same prop name across the whole MUI stack.
If we drop it for the column menu, people will have to use components on some places and slots on other, to use camel case on some places and not on others.

@MBilalShafi
Copy link
Member Author

MBilalShafi commented Feb 22, 2023

Was the decision to entirely drop the components prop discussed during a meeting ?

It wasn't discussed, but the motivation behind going ahead with slots only for Column Menu API is to:

  1. Keep the API and documentation simple and unified
  2. Previously we did not have any such API there (we only introduced it in alpha) so there is very rare to no chances people are using it widely (or there could be breaking changes) so it could be a good opportunity to port the API already to slots pattern.
  3. Encourage users to start using slots pattern already

For me the main argument in favor of keeping both was to let people use the same prop name across the whole MUI stack.

That's a very good point, but shouldn't the end goal be to port the users to use slots, will some users complain if we only support slots pattern in new places where we provide similar APIs and keep both deprecated components and slots in Grid rootProps 🤔

@flaviendelangle
Copy link
Member

That's a very good point, but shouldn't the end goal be to port the users to use slots

Yes, but until the MUI Core library support slots, people have to use components at least on those packages.
My goal was to say that until slots is available everywhere, we keep components as well to make sure that people don't have to learn two nomenclature.

And then when the MUI Core libraries support slots, we can deprecate our components props and advertise about the codemod to let people easily migrate from one to the other.

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.

I don't see any problem with already supporting only slots for the column menu. For components the scenario is different, users may already have customizations using this prop, but the column menu was recently added.

Should we consider this a breaking change? No need to update the migration guide, only the CHANGELOG for users already using the new menu.

docs/data/data-grid/column-menu/column-menu.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

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

@DanailH
Copy link
Member

DanailH commented Mar 15, 2023

Should we move to merge this?

@MBilalShafi
Copy link
Member Author

MBilalShafi commented Mar 24, 2023

For components the scenario is different, users may already have customizations using this prop, but the column menu was recently added.

Alright, I will then assume then if we need to open the interface of slots/slotProps to a new component similar to ColumnMenu (before v7), we won't add deprecated components/componentsProps to it, we only keep supporting components/componentsProps in rootProps as this is an opinionated one and the tradeoff seems ok to me keeping in mind the likely user-base using this (or probable new) APIs.

Should we move to merge this?

I guess we can, we already see use-cases where it could be helpful and we should finalize the API soon.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2023
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Signed-off-by: Bilal Shafi <bilalshafidev@gmail.com>
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! v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants