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

[Joy UI] Add CircularProgress component #33869

Merged
merged 109 commits into from Sep 12, 2022
Merged

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Aug 9, 2022

Done:

  • Initial implementation
  • Playground demo
  • Props: color, size, variant: 'indeterminate' | 'determinate', etc; To be more discussed with Jun and Danilo

UPDATE:

  • ✅ Iterate on demos/implementation with Jun and Danilo's feedback
  • ✅ Add unit tests
  • ✅ Add typescript test

Preview: https://deploy-preview-33869--material-ui.netlify.app/joy-ui/react-circular-progress/

@hbjORbj hbjORbj added new feature New feature or request component: CircularProgress The React component package: joy-ui Specific to @mui/joy labels Aug 9, 2022
@hbjORbj hbjORbj self-assigned this Aug 9, 2022
@mui-bot
Copy link

mui-bot commented Aug 9, 2022

Details of bundle changes

@mui/joy: parsed: +2.00% , gzip: +1.83%

Generated by 🚫 dangerJS against 5807b80

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet, looking awesome, Benny! A question: we mention determinate and indeterminate in the introduction-how would I go about it to controlling the component so I have one or the other? Is that done through a prop?

Also, maybe we should document size and color (even though they're on the Playground there already) just for the sake of consistency?! Other components have those sections too.

@hbjORbj
Copy link
Member Author

hbjORbj commented Aug 9, 2022

@danilo-leal

A question: we mention determinate and indeterminate in the introduction-how would I go about it to controlling the component so I have one or the other? Is that done through a prop?

Yes, through a prop called variant at the moment, but I think we should change the name to something else to reduce confusion since variant is used often in other Joy components for theme values. Maybe a prop called determinate that can be either true or false? It can be false by default.

Also, maybe we should document size and color (even though they're on the Playground there already) just for the sake of consistency?! Other components have those sections too.

I agree. Will do that!

@danilo-leal
Copy link
Contributor

I dig the idea of a boolean determinate prop! 👌

@hbjORbj
Copy link
Member Author

hbjORbj commented Aug 11, 2022

@danilo-leal I dig the idea of a boolean determinate prop! 👌

Done! Can you suggest some ideas for demos?

@siriwatknp
Copy link
Member

siriwatknp commented Aug 15, 2022

@danilo-leal @hbjORbj Does it make sense to not follow the Material Design look and feel?

Here is a simpler version using div + border:

Screen.Recording.2565-08-15.at.12.12.48.mov

https://mui.com/joy-ui/react-link/#common-examples


Variant

Let's add the global variant to CircularProgress. I think it is useful for composition:

<Button startIcon={<CircularProgress variant="soft" />}>
  • The ring should use theme.vars.palette[ownerState.color]['${ownerState.variant}Bg']
  • The progress should use theme.vars.palette[ownerState.color]['${ownerState.variant}Color']

Children

I think it should be possible to place a child center by default. It is quite a common use case. https://www.pinterest.com/pin/480337116518704364/

Please add a demo for this as well.

CSS variables

For whatever design the CircularProgress looks like, it should contain these generic variables:

  • let's add --CircularProgress-size that can be customized by the parent
    {
       width: 'var(--CircularProgress-size, 32px)',
    }
  • let's add --CircularProgress-thickness that can be customized by the parent as well.
  • I think --CircularProgress-speed to control how fast it moves is worth trying.

Please add a CSS variables demo.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2022
@hbjORbj
Copy link
Member Author

hbjORbj commented Aug 26, 2022

Hey, Jun @siriwatknp , I addressed all your feedback except one: I am not sure how to go about implementing determinate prop. Can you help me with that?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 28, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Aug 29, 2022

@hbjORbj Looks like my proposed solution, using a border, would not work for Determinate progress. Seems like using svg with stroke-dasharray and stroke-dashoffset is the best approach so far.

@siriwatknp
Copy link
Member

@danilo-leal
Copy link
Contributor

Sweet, thanks @hbjORbj, it looks good now! I see we're centralizing them using specific values for each CircularProgress size. Have you verified if that works out if you change the icon's size?

@hbjORbj
Copy link
Member Author

hbjORbj commented Sep 8, 2022

@danilo-leal

I verified that it works out if we change the component (circular progress)'s size, but not the icon's size. Just confirmed that it breaks with varying the icon's size. Hmm, I am not sure how to best about this. Any idea @siriwatknp Jun?

@siriwatknp
Copy link
Member

@danilo-leal

I verified that it works out if we change the component (circular progress)'s size, but not the icon's size. Just confirmed that it breaks with varying the icon's size. Hmm, I am not sure how to best about this. Any idea @siriwatknp Jun?

FYI, I am trying alternative way, will share soon.

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

This is looking great now - awesome work you two @hbjORbj & @siriwatknp as always!

@siriwatknp
Copy link
Member

siriwatknp commented Sep 9, 2022

Changes from me

  • fix the rotation degree to let the progress starts from the top

  • rename the slot to be more meaningful from circle1, circle2 to track, progress (similar to Switch, Slider)

  • Reduce the default size values to match other components, e.g. IconButton, Button

  • Make the svg absolute so that the children of the CircularProgress can stay at the center using flexbox.

  • update the outline styles to be consistent with other components (using border):
    Screen Shot 2565-09-09 at 15 20 30

  • add some more useful variables: --CircularProgress-circulation, --CircularProgress-linecap.

  • fine-tune the track and progress stroke:

    Screen.Recording.2565-09-09.at.15.26.02.mov
  • Make the CircularProgress's size adaptable to Button, IconButton, and Link.

@hbjORbj
Copy link
Member Author

hbjORbj commented Sep 9, 2022

Brilliant Jun 🤩🤩🤩 Thanks!!!

siriwatknp and others added 2 commits September 12, 2022 10:50
Co-authored-by: danilo leal <67129314+danilo-leal@users.noreply.github.com>
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
@hbjORbj hbjORbj merged commit 6d5558a into mui:master Sep 12, 2022
@mbrookes
Copy link
Member

Sorry for commenting after merge, I don't have time these days to follow progress on GitHub, but happened to spot this while looking for something else.

A few thoughts (I haven't read the discussion here if they've already been covered):

  • What do you think about slowing down the default speed? 0.5s seems a bit frenetic. 1s feels much more calm. This could perhaps be configurable by a prop?
  • Outlined feels like it should have an inner outline, so that the track itself is outlined.
  • The color of the spinner doesn't appear to match the color selected in the Playground.
  • It might be nice to have (an option of?) a linear gradient on the spinner so that it fades towards the tail, along the lines of this:
    image
  • It isn't possible to adjust CSS variables with the keyboard. (On which note, when can we expect @siriwatknp's number input to be migrated to MUI? 😁 It's the most requested component! )

@siriwatknp
Copy link
Member

What do you think about slowing down the default speed? 0.5s seems a bit frenetic. 1s feels much more calm. This could perhaps be configurable by a prop?

No problem!

Outlined feels like it should have an inner outline, so that the track itself is outlined.

Agree, that's a better outlined.

The color of the spinner doesn't appear to match the color selected in the Playground.

Yeah, the swatch should follow the variant. Will fix it in a separate PR.

It might be nice to have (an option of?) a linear gradient on the spinner so that it fades towards the tail, along the lines of this:

Good suggestion but I think it will be a customization example, not a built-in style. Will try to add it soon.

It isn't possible to adjust CSS variables with the keyboard. (On which note, when can we expect @siriwatknp's number input to be migrated to MUI? 😁 It's the most requested component! )

Yeah, I think we should start the NumberInput on MUI Base soon. cc @michaldudak

@michaldudak
Copy link
Member

There is a growing list of components we should create in MUI Base :) Let's set priorities on the next product meeting.

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: CircularProgress The React component new feature New feature or request package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants