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

[TreeView] Use useDefaultProps instead of useThemeProps #14772

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 30, 2024

Use the new API introduced by the core team.

We need to bump @mui/material deps to use 5.16.0 or above in order to support it, which makes this a breaking change.

I'm opening this PR to check with the core if the approach is good before preparing the one for the pickers to merge it at the beginning of the alpha.

@flaviendelangle flaviendelangle self-assigned this Sep 30, 2024
@flaviendelangle flaviendelangle requested review from siriwatknp and removed request for mnajdova September 30, 2024 08:27
@flaviendelangle flaviendelangle added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Sep 30, 2024
@mui-bot
Copy link

mui-bot commented Sep 30, 2024

Deploy preview: https://deploy-preview-14772--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d5652c8

@siriwatknp
Copy link
Member

@flaviendelangle I got this error, seems like there is a wrong usage of alpha()

Error: /Users/siriwatknp/practice-repos/tree-view-pigment-css/node_modules/@mui/x-tree-view/TreeItem2DragAndDropOverlay/TreeItem2DragAndDropOverlay.js: MUI: Unsupported `var(--mui-palette-primary-dark, #1565c0)` color.
The following formats are supported: #nnn, #nnnnnn, rgb(), rgba(), hsl(), hsla(), color().

It comes from TreeItem2DragAndDropOverlay, it should be:

…
    style: {
      marginLeft: 'calc(var(--TreeView-indentMultiplier) * var(--TreeView-itemDepth))',
      borderRadius: theme.shape.borderRadius,
-      backgroundColor: alpha((theme.vars || theme).palette.primary.dark, 0.15)
+      backgroundColor: alpha(theme.palette.primary.dark, 0.15)
    }

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

@flaviendelangle flaviendelangle added the on hold There is a blocker, we need to wait label Sep 30, 2024
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.

To note that we have mui/material-ui#43443 that advocates for removing this API.

@flaviendelangle
Copy link
Member Author

@oliviertassinari we'll wait for the core to settle on an API then

But I don't understand what you are proposing to allow people to set default value to their props globally.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2024

The TL:DR of the thought process behind the issue I linked was:

  1. Those MUI System APIs are not RSC compatible. E.g. you can't have a <Grid> with a different gap value today, you can't create a custom server side Breadcrumb and get this outcome, it doesn't work, but it should.
  2. We can fix RSC without them, E.g. we use a global value, no different to Pigment CSS having a global theme, but also works ith Emotion.
  3. Once RSC is fixed, they seem to be unnecessary complexity as well as migration pain
  4. So why not fix RSC and remove them?

If each step of the reasoning is right (to be confirmed), then I think that no MUI X propagation makes the most sense. Sure, we could propagate those in MUI X, if we are happy to refactor it again in the future, but then, why not spend that same time fixing the root?

@flaviendelangle
Copy link
Member Author

So you are planning to remove the ability to define default value to the props globally?
Or do you have another approach that is RSC compatible? (which would I guess be limited to stringifiable props).

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2024

So you are planning to remove the ability to define default value to the props globally?
Or do you have another approach that is RSC compatible? (which would I guess be limited to stringifiable props).

@flaviendelangle Default global props sound important.

I think stringifiable props was great to POC, but if we can duplicate the theme, have it as a JavaScript object, define it once in the server-side bundle, and once in the client-side bundle then this seems to be a much better solution. I spent 1hr on this to prove the previous points: https://github.com/oliviertassinari/test-theme. It seems to work.

@flaviendelangle
Copy link
Member Author

OK
And for you the API used would be the one of v5 with a ThemeProvider and a useThemeProps?

@oliviertassinari
Copy link
Member

@flaviendelangle I would imagine so. Assuming there is not benefits to change those APIs.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 8, 2024
Copy link

github-actions bot commented Oct 8, 2024

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

@brijeshb42
Copy link

@oliviertassinari From what I know, any prop can be set through ThemeProvider/DefaultPropsProvider. If it is a serializable value, then it will work with RSC but if it isn't it won't. My main concern is that we allow anything to be overridden through the provider and we don't know what. So here, it seems better to be use the useDefaultPropsProvider. If it is specific to theme values, then useTheme() would make sense.
@flaviendelangle Let me know if I am missing something.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 30, 2024

@brijeshb42 See https://github.com/oliviertassinari/test-theme the theme doesn't need to be serializable for useTheme to work with RSC. Can we:

  • Add useTheme support
  • Remove DefaultPropsProvider

before going to beta?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! on hold There is a blocker, we need to wait PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants