Skip to content

[Card] Warn on raised + outlined#24648

Merged
eps1lon merged 5 commits into
mui:nextfrom
sumarlidason:raised_elevation_outlined
Jan 28, 2021
Merged

[Card] Warn on raised + outlined#24648
eps1lon merged 5 commits into
mui:nextfrom
sumarlidason:raised_elevation_outlined

Conversation

@sumarlidason

@sumarlidason sumarlidason commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Adds warnings when combining elevation related props with incompatible variants.

Closes #24629

@mui-pr-bot

mui-pr-bot commented Jan 27, 2021

Copy link
Copy Markdown

Details of bundle changes

Generated by 🚫 dangerJS against 0010199

Comment thread packages/material-ui/src/Button/Button.js Outdated
@oliviertassinari oliviertassinari changed the title warn on elevation+outlined [Card] Warn on elevation+outlined Jan 27, 2021
@oliviertassinari oliviertassinari changed the title [Card] Warn on elevation+outlined [Card] Warn on raised + outlined Jan 27, 2021
@oliviertassinari oliviertassinari added scope: card Changes related to the card. docs Improvements or additions to the documentation. labels Jan 27, 2021
Comment thread docs/translations/api-docs/card/card-de.json Outdated
Comment thread packages/material-ui/src/Card/Card.js Outdated
Comment thread packages/material-ui/src/Paper/Paper.js Outdated
  - revert manual changes to localization
  - use present tense within proptypes warnings

@oliviertassinari oliviertassinari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The logic looks declarative enough to not require a test case

@sumarlidason

Copy link
Copy Markdown
Contributor Author

@oliviertassinari thanks for the hand holding 👋

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Forgot to submit the review. Will follow-up.

elevation: chainPropTypes(PropTypes.number, (props) => {
if (props.elevation > 0 && props.variant === 'outlined') {
return new Error(
'Material-UI: Combining `elevation={>0}` with `variant="outlined"` has no effect.',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'Material-UI: Combining `elevation={>0}` with `variant="outlined"` has no effect.',
`Material-UI: Combining \`elevation={${props.elevation}}\` with \`variant="outlined"\` has no effect. Either use a different `variant` or use \`elevation={0}\`.`,

I understand what you're trying to tell with >0 but it's not valid JS and including the actual elevation makes it easier to find.

And then we should add a test for that. You can check out the existing tests using PropTypes.checkPropTypes to find out how to test this kind of code.

@eps1lon
eps1lon merged commit b38aa12 into mui:next Jan 28, 2021
@eps1lon

eps1lon commented Jan 28, 2021

Copy link
Copy Markdown
Member

@sumarlidason Thanks!

natac13 pushed a commit to natac13/material-ui that referenced this pull request Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: card Changes related to the card.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Card] undocumented mutually exclusive props

4 participants