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

[Card] Migrate CardMedia to emotion #24625

Merged
merged 5 commits into from
Jan 27, 2021
Merged

Conversation

natac13
Copy link
Contributor

@natac13 natac13 commented Jan 26, 2021

#24405

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 26, 2021

@material-ui/core: parsed: +0.14% , gzip: +0.08%

Details of bundle changes

Generated by 🚫 dangerJS against 949f0ed

@oliviertassinari oliviertassinari changed the title [CardMedia] Migrate to emotion [Card] Migrate CardMedia to emotion Jan 26, 2021
@oliviertassinari oliviertassinari added the component: card This is the name of the generic UI component, not the React module! label Jan 26, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks much better! Last two comments and I believe we are good to go :)

packages/material-ui/src/CardMedia/CardMedia.js Outdated Show resolved Hide resolved
packages/material-ui/src/CardMedia/CardMedia.js Outdated Show resolved Hide resolved
@@ -79,7 +79,7 @@ describe('<CardMedia />', () => {

it('warns when neither `children`, nor `image`, nor `src`, nor `component` are provided', () => {
expect(() => {
PropTypes.checkPropTypes(CardMedia.Naked.propTypes, { classes: {} }, 'prop', 'MockedName');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was failing with propTypes cannot be read from undefined.
I looked at other Mui component with this test but none of the others have been converted yet, therefore I was not sure if this was the correct approach.
Removing the Naked got the tests passing.

Copy link
Member

Choose a reason for hiding this comment

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

That's the correct solution 👍

Copy link
Member

Choose a reason for hiding this comment

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

A bit of clarification Naked was added by the withStyles utility, we don't use it anymore, so this is correct :)

packages/material-ui/src/CardMedia/CardMedia.js Outdated Show resolved Hide resolved
@mnajdova mnajdova merged commit 74dcb12 into mui:next Jan 27, 2021
xs9627 added a commit to xs9627/material-ui that referenced this pull request Jan 27, 2021
* next: (34 commits)
  [Tab] Migrate to emotion (mui#24651)
  [TextField] Migrate Input component to emotion (mui#24638)
  [ImageList] Migrate ImageListItemBar to emotion (mui#24632)
  [CircularProgress] Migrate to emotion (mui#24622)
  [DataTable] Add example in docs for data table (mui#24428)
  [Card] Migrate CardActionArea to emotion (mui#24636)
  [core] Fix `next` using stale pages (mui#24635)
  [List] Migrate ListItemIcon to emotion (mui#24630)
  [ClickAwayListener] Fix `children` and `onClickAway` types (mui#24565)
  [docs] Include in docs directive to silence `eslint` erroneous warning (mui#24644)
  [Fab] Migrate to emotion (mui#24618)
  [TextField] Migrate FilledInput to emotion (mui#24634)
  [Card] Migrate CardHeader to emotion (mui#24626)
  [Card] Migrate CardMedia to emotion (mui#24625)
  [ImageList] Migrate ImageListItem to emotion (mui#24619)
  [website] Add vision block (mui#24603)
  [docs] Add sorting section (mui#24637)
  [TextField] Prepare removal of labelWidth prop (mui#24595)
  [Dialog] Migrate DialogTitle to emotion (mui#24623)
  [ImageList] Migrate to emotion (mui#24615)
  ...
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants