Open json2d opened 1 year ago
Hey @json2d, thanks for the report!
I think this is a bug because we're coupling two different things inside the variant
prop:
"rectangular"
, "rounded"
, "circular"
"text"
sizing, which deals with its heightThis makes it impossible to match text sizing with "rectangular"
border radius, for example.
We should probably separate these into two different props.
I would like to know your thoughts on this @siriwatknp cc: @mui/core
@DiegoAndai we would like to pick this up
Hey @gitstart! I would like to have a clear path forward and wait for what @mui/core thinks before someone starts to work on it. I'm not sure if my proposal is the best option or if there are other ideas, so feel free to chime into the discussion of how should this problem be solved.
It makes sense what you're proposing, @DiegoAndai. There should be two separate props to control the sizing and the shape. Note that it will be a breaking change, so we can consider this for v6.
@michaldudak what do you think about deprecating variant
and introducing shape
and sizing
in v5, making it backward compatible, and then removing variant
for v6?
:+1: yup, this should work.
Great, I'll mark this as ready to take with these changes to be implemented
variant
prop should be marked as deprecated. Its functionality should be exactly the same, and to ensure it we should add variant
tests as they're missing right now. The deprecation message should explain that shape
and sizing
should be used instead.shape
prop should be added with values rectangular
, rounded
, and circular
. sizing
prop should be added with values text
and box
. The text
option will scale the skeleton's height to match the font size, the box
option will match its height to the child's bounding box (height = auto
).The shape
and sizing
props shouldn't have defaults, as the variant default should be maintained, but they should be added for v6. Let's leave a TODO: v6
message to add defaults.
If shape
and sizing
are not provided, the variant
behavior should be maintained, for example
variant="rectangle"
Should render a rectangle skeleton adjusted to the bounding box.
If provided, the shape
and sizing
props should override the variant
prop, for example:
variant="rectangle"
shape="rectangle"
sizing="text"
Should render a rectangle skeleton adjusted to text.
@gitstart if you or anyone else would like to take this that would be awesome 🚀. Let us know if you start working on it.
Thanks for the update @DiegoAndai, we'll be picking this up.
just trying to use the Skeleton inferring dimensions feature w/ variants
@json2d What's not working with <Skeleton variant="text" />
? And why not go with this in your reproduction?
function TypographyDemo(props) {
const { loading = false } = props;
return (
<div>
{variants.map((variant) => (
<Typography component="div" key={variant} variant={variant}>
{loading ? <Skeleton variant="text" sx={{ borderRadius: 0 }} /> : variant}
</Typography>
))}
</div>
);
}
For context, the component was designed with the assumption that variant="circular"
and variant="rectangular"
have no use cases for a skeleton that represents text, as Joy UI did: https://mui.com/joy-ui/react-skeleton/.
I'm reopening for https://github.com/mui/material-ui/pull/38483#issuecomment-1732547880. I think we have room to improve the docs.
We reverted (https://github.com/mui/material-ui/pull/39156) the PR (https://github.com/mui/material-ui/pull/38483) that fixed this issue as it had room for improvement. I'd like to continue discussing the possible solution to this issue here.
Bundling border-radius and dimensions into one prop is not a perfect API design, as it can be confusing. On the other hand, it's not a massive issue as it has a workaround using the sx prop (https://github.com/mui/material-ui/issues/38190#issuecomment-1732544018), and we've come this far with it, so as @oliviertassinari mentions in https://github.com/mui/material-ui/pull/38483#issuecomment-1732547880, it seems to be something that affects a small percentage of users.
The options I see here:
variant
is meant to be used for specific cases, and provide an example for changing the border-radius of the text variant so users don't get into this particular confusionThere's a case for both, but considering that this hasn't affected many users and is achievable via the sx prop, I now lean towards 1.
Would you have any thoughts to share on this? Is there an option I'm not seeing?
cc: @mnajdova
@oliviertassinari i think the example you provided is a reasonable workaround to make it round, though the hope was to make use the inferencing, which variant="text"
does not support
just trying to use the Skeleton inferring dimensions feature w/ variants
@json2d What's not working with
<Skeleton variant="text" />
? And why not go with this in your reproduction?function TypographyDemo(props) { const { loading = false } = props; return ( <div> {variants.map((variant) => ( <Typography component="div" key={variant} variant={variant}> {loading ? <Skeleton variant="text" sx={{ borderRadius: 0 }} /> : variant} </Typography> ))} </div> ); }
For context, the component was designed with the assumption that
variant="circular"
andvariant="rectangular"
have no use cases for a skeleton that represents text, as Joy UI did: https://mui.com/joy-ui/react-skeleton/.
This makes the most sense to me. We could add a note about adjusting border-radius (as an example) at the end of the variant="text"
example.
It seems that there's agreement on going for the workaround of using the sx prop to adjust the border radius for the text variant. I'm adding the ready-to-take label with this new requirement:
We should do this for Material and Joy at the same time. Do you agree, @siriwatknp?
It seems that there's agreement on going for the workaround of using the sx prop to adjust the border radius for the text variant. I'm adding the ready-to-take label with this new requirement:
- Provide an explanation and example on the docs for changing the border radius of the text variant, so users don't get confused trying to use the text variant with a different border radius.
We should do this for Material and Joy at the same time. Do you agree, @siriwatknp?
Agree!
Duplicates
Latest version
Steps to reproduce 🕹
Link to live example: https://codesandbox.io/s/clever-resonance-wl4snj?file=/Demo.js:454-517
Steps:
variant="rounded"
orvariant="rectangular"
to<Skeleton />
Current behavior 😯
Skeleton inferring dimensions missing padding when variant prop is set:
Expected behavior 🤔
Skeleton inferring dimensions not missing padding when variant prop is set:
Context 🔦
just trying to use the Skeleton inferring dimensions feature w/ variants
Your environment 🌎
skipped
@mui/envinfo
output - using Codesandbox w/ Chrome: 114.0.5735.248