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

[docs] Migrate Card demos to emotion #25557

Merged

Conversation

vicasas
Copy link
Member

@vicasas vicasas commented Mar 29, 2021

Related to #16947

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 29, 2021

No bundle size changes

Generated by 🚫 dangerJS against f6475bf

@vicasas
Copy link
Member Author

vicasas commented Mar 29, 2021

@oliviertassinari For the example Complex Interaction use the same technique applied for one of the examples of FormControlLabel (Example) to handle the dynamic classes that depend on a boolean but here when using it it throws an error in the warning type console

image

Warning: Received false for a non-boolean attribute expand.
If you want to write it to the DOM, pass a string instead: expand="false" or expand={value.toString()}.
If you used to conditionally omit it with expand={condition && value}, pass expand={condition ? value : undefined} instead.

I wonder if this approach is the correct one to tackle these cases? And why in this scenario does this error occur? I tried in different ways but it always ended with the warning in the console.

UPDATE

Reviewing the FormControlLabel in more detail, I realize that this component receives a prop named checked, maybe that's why I don't jump that error there.

@oliviertassinari oliviertassinari added component: card This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Mar 30, 2021
@oliviertassinari
Copy link
Member

@vicasas we should prevent the prop to be spread on the DOM node in this case.

@oliviertassinari
Copy link
Member

@vicasas I have pushed a few commits on side improvements I could notice. If you could fix the leaking prop, I think that it will be perfect, we could move forward with the changes.

@vicasas
Copy link
Member Author

vicasas commented Mar 30, 2021

@oliviertassinari I have 3 options to address these cases:

  1. Using api styled
interface ExpandMoreProps extends IconButtonProps {
  expand: boolean;
}

const ExpandMore = styled((props: ExpandMoreProps) => {
  const { expand, ...rest } = props; // unstructuring the props so as not to send the  `expand` prop to the DOM node
  return <IconButton {...rest} />;
})(({ theme, expand }) => ({
  transform: !expand ? 'rotate(0deg)' : 'rotate(180deg)',
  marginLeft: 'auto',
  transition: theme.transitions.create('transform', {
    duration: theme.transitions.duration.shortest,
  }),
}));
  1. Using sx prop
sx={{
    transform: !expanded ? 'rotate(0deg)' : 'rotate(180deg)',
    marginLeft: 'auto',
    transition: (theme) =>
    theme.transitions.create('transform', {
       duration: theme.transitions.duration.shortest,
    }),
}}
  1. Using api atyled and sx prop. (1 and 2)

I certainly liked the previous approach using styled without having to destroy because I think it was cleaner, but since it gives the error for sending the prop to the DOM some of these options should work for us.

What do you think?

@oliviertassinari
Copy link
Member

@vicasas I guess 1 is good enough?

@vicasas
Copy link
Member Author

vicasas commented Mar 30, 2021

@oliviertassinari Exactly, I still lean towards 1. The only noise it makes is to leave the prop unused in the destructuring, in eslint rules it can give problems for having an unused prop

Finally each developer will choose their way of approaching this, but I think it is good to guide them to use option 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: card This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants