[CircularProgress] Migrate to emotion by natac13 · Pull Request #24622 · mui/material-ui

natac13 · 2021-01-25T18:09:04Z

I have followed (at least) the PR section of the contributing guide.

#24405

Lol still not perfect.

mui-pr-bot · 2021-01-25T18:11:24Z

@material-ui/core: parsed: +0.39% , gzip: +0.26%
@material-ui/lab: parsed: +0.42% , gzip: +0.42%

Details of bundle changes

Generated by 🚫 dangerJS against e29d24d

packages/material-ui/src/CircularProgress/CircularProgress.js

mnajdova · 2021-01-26T16:57:38Z

packages/material-ui/src/CircularProgress/CircularProgress.js

  /* Styles applied to the `circle` svg path if `variant="indeterminate"`. */
-  circleIndeterminate: {
+  ...(styleProps.variant === 'indeterminate' && {
    animation: '$circular-dash 1.4s ease-in-out infinite',


Suggested change

animation: '$circular-dash 1.4s ease-in-out infinite',

animation: 'circular-dash 1.4s ease-in-out infinite',

Isn't this the correct?

mnajdova · 2021-01-26T16:58:13Z

packages/material-ui/src/CircularProgress/CircularProgress.js

  /* Styles applied to the root element if `variant="indeterminate"`. */
-  indeterminate: {
+  ...(styleProps.variant === 'indeterminate' && {
    animation: '$circular-rotate 1.4s linear infinite',


Suggested change

animation: '$circular-rotate 1.4s linear infinite',

animation: 'circular-rotate 1.4s linear infinite',

mnajdova

Actually, can we follow how the keyframes are created in the TouchRipple component? https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/ButtonBase/TouchRipple.js#L14 Also note that we will have to convert the styles to the CSS syntax when using keyframes. See - https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/ButtonBase/TouchRipple.js#L62 It is unfortunate, but it is the only way we can ensure that both emotion & styled-components will work correctly. I can help out in the end to test out that both scenarios work.

natac13 · 2021-01-26T17:05:22Z

No problem. Ill make the changes. And reach out if I get stuck. 🤞🏻

mnajdova · 2021-01-26T17:06:03Z

No problem. Ill make the changes. And reach out if I get stuck. 🤞🏻

Perfect! Thank you 🙏

natac13 · 2021-01-26T18:27:39Z

I believe I have converted styling to css successfully. However, I am still getting the last check of testRootClass from describeConformanceV5 failing.

Sorry, I am not sure why.

packages/material-ui/src/CircularProgress/CircularProgress.js

mnajdova · 2021-01-26T18:47:54Z

I believe I have converted styling to css successfully. However, I am still getting the last check of testRootClass from describeConformanceV5 failing.

Sorry, I am not sure why.

Take a look on the last comments, should fix the failing test:


  1) <CircularProgress />
       Material-UI component API
         applies the root class to the root component if it has this class:
     AssertionError: expected '[object Object]' to equal null
      at Context.it (test/utils/describeConformance.js:166:85)

The CSS styles looks good from glance, will test them in more details tomorrow (sorry don't have the energy to od it right now :) ). Thanks for taking a look at this 👍

mnajdova · 2021-01-27T14:32:15Z

@natac13 I've pushed some fixes with e6f14f3 finally it's working (tested with both emotion & styled-components) I need to also change the styles for the determinate variant not to use class name and I believe we are good to go.

mnajdova · 2021-01-27T14:36:19Z

Ok, found the isseue, I was doing

  transition: ${({ styleProps, theme }) => styleProps.variant === 'determinate' && theme.transitions.create('transform')};

instead of

  transition: ${({ styleProps, theme }) =>
    styleProps.variant === 'determinate' ? theme.transitions.create('transform') : undefined};

Everything should be good now :)

mnajdova

Good job on this one, it was more tricky than the others :)

natac13 · 2021-01-27T15:03:08Z

Thanks, glad to be able to help.

transition: ${({ styleProps, theme }) =>
styleProps.variant === 'determinate' ? theme.transitions.create('transform') : undefined};

I was not sure I could do this. lol I had something like this in the start but didn't think I could leave undefined

Thanks for the help

* 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) ...

Migrate CircularProgress

ec9e217

oliviertassinari added the component: CircularProgress The React component. label Jan 25, 2021

fixes styles

12dc24f

oliviertassinari mentioned this pull request Jan 25, 2021

Migrate all components to emotion #24405

Closed

github-actions bot mentioned this pull request Jan 25, 2021

Update snapshots for pr/24622 eps1lon/mui-scripts-incubator#1756

Closed

ran yarn docs:api on circular-progress emotion

2758988

mnajdova reviewed Jan 26, 2021

View reviewed changes

mnajdova requested changes Jan 26, 2021

View reviewed changes

Update styles to css for CircularProgress emotion

77fe6c6

mnajdova reviewed Jan 26, 2021

View reviewed changes

packages/material-ui/src/CircularProgress/CircularProgress.js Outdated Show resolved Hide resolved

mnajdova reviewed Jan 26, 2021

View reviewed changes

packages/material-ui/src/CircularProgress/CircularProgress.js Outdated Show resolved Hide resolved

natac13 and others added 3 commits January 26, 2021 18:46

fix failing test for CircularProgress emotion

8c99fd2

add sx to framer ignore CircularProgress

ad5a0c6

fixed on CSS syntax + decreaded specificity

e6f14f3

final fixes

c670e5f

mnajdova approved these changes Jan 27, 2021

View reviewed changes

mnajdova added 2 commits January 27, 2021 16:17

fixed tests

3cd17cd

lint fixes

e29d24d

mnajdova merged commit 93b6eb9 into mui:next Jan 27, 2021

mnajdova mentioned this pull request Jan 28, 2021

[Skeleton] Migrate to emotion #24652

Merged

1 task

natac13 added a commit to natac13/material-ui that referenced this pull request Jan 30, 2021

[CircularProgress] Migrate to emotion (mui#24622)

57486e0

oliviertassinari added the scope: progress Changes related to the progress. label Nov 28, 2021

	animation: '$circular-dash 1.4s ease-in-out infinite',
	animation: 'circular-dash 1.4s ease-in-out infinite',

	animation: '$circular-rotate 1.4s linear infinite',
	animation: 'circular-rotate 1.4s linear infinite',

Uh oh!

Conversation

natac13 commented Jan 25, 2021

Uh oh!

mui-pr-bot commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mnajdova Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

mnajdova Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

mnajdova Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

natac13 commented Jan 26, 2021

Uh oh!

mnajdova commented Jan 26, 2021

Uh oh!

natac13 commented Jan 26, 2021

Uh oh!

Uh oh!

Uh oh!

mnajdova commented Jan 26, 2021

Uh oh!

mnajdova commented Jan 27, 2021

Uh oh!

mnajdova commented Jan 27, 2021

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

natac13 commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mui-pr-bot commented Jan 25, 2021 •

edited

Loading