mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[TreeView] Use `useDefaultProps` instead of `useThemeProps`

Open flaviendelangle opened this issue 1 year ago • 11 comments

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 avatar Sep 30 '24 08:09 flaviendelangle

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

Generated by :no_entry_sign: dangerJS against d5652c8f8f772e9bdf54c4ee6438c5f16d1c235d

mui-bot avatar Sep 30 '24 08:09 mui-bot

@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)
    }

siriwatknp avatar Sep 30 '24 09:09 siriwatknp

@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.

flaviendelangle avatar Sep 30 '24 12:09 flaviendelangle

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?

oliviertassinari avatar Sep 30 '24 12:09 oliviertassinari

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 avatar Sep 30 '24 12:09 flaviendelangle

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.

oliviertassinari avatar Sep 30 '24 14:09 oliviertassinari

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

flaviendelangle avatar Sep 30 '24 15:09 flaviendelangle

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

oliviertassinari avatar Sep 30 '24 15:09 oliviertassinari

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

github-actions[bot] avatar Oct 08 '24 10:10 github-actions[bot]

@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.

brijeshb42 avatar Oct 30 '24 04:10 brijeshb42

@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?

oliviertassinari avatar Oct 30 '24 18:10 oliviertassinari

Closing the PR because the Olivier / core discussion is not moving forward :/

flaviendelangle avatar Jan 31 '25 14:01 flaviendelangle

IMHO, the result of this discrepancy doesn't look good. 🙈 If Core is using the new API, so should X counterpart libraries, otherwise, this is highly misleading. 🤷


Commenting because I saw people trying to use Pigment.... https://github.com/mui/mui-x/issues/16438#issuecomment-3000629217

LukasTy avatar Jun 24 '25 14:06 LukasTy