-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Fix DataTable.tsx demo in v4 #27196
Conversation
Details of bundle changes.Comparing: 4fe0df5...1a4e85b Details of page changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the version we depend on and handle the breaking changes? See mui/mui-x#2039
18852ec
to
b832620
Compare
2e9bd15
to
3727579
Compare
Blocked by mui/mui-x#2108 and mui/mui-x#2117 |
@oliviertassinari Do we accept fixes for the v4 docs now? |
@eps1lon This is for the docs of MUI X that has its ongoing development with v4. It's not mean to fix the docs of the core components. |
It's opened in mui-org/material-ui targetted at |
@eps1lon I meant that this PR fixes the docs of the data grid component in v4. We don't work on the docs of the core components in v4. The policy for the core components is in https://github.com/mui-org/material-ui#installation I wouldn't say this change goes against it. At least, it depends on how we frame it. For me, this policy was introduced so that: 1. we incentive developers to upgrade to @material-ui/core v5, 2. we don't waste time on a duplicated effort (doing the same changes twice). |
This definitely goes against existing policy. We want to avoid diverging |
Ok, a different interesting motivation. From a divergence, do you mean the change of Node.js version? Or I guess more about any changes that we might do. What would be the negative implications of a divergence? Regarding why we might want to merge this. The demonstrated code is not working: copy and paste the source, install the latest version: 💥. In the X packages, we have optimized for the adoption of the data grid, so we picked core v4 rather than v5. In the core packages, we have optimized for the adoption of v5. +1 I would for making an exception on this. If we don't fix the demo, I think that it's important we remove the demo or explain which version to use. It's not like with the other packages, where developers can use the latest version of a release line and it goes OK. I actually wonder about the timing. In 2 months or so, this demo will no longer be on the TLD but on v4.material-ui.com, along with all its demo of the data grid. We will no longer have documentation of the data grid on the main domain. cc @mui-org/x We will have to migrate all the docs environments to core v5. Maybe we should work on it two weeks before the stable release date. I have added a new items in mui/mui-x#1902. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed two commits to align the TS config between master
and next
. 2186dce fixes the same problem related in mui/mui-x#2106. I'm OK with pushing this one forward.
@m4theushw I think that we should revert 2186dce. It's only because we don't skip the |
This reverts commit 2186dce.
a9694a8
to
1a4e85b
Compare
Fix mui/mui-x#2039
Preview: https://deploy-preview-27196--material-ui.netlify.app/components/tables/#data-table