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

refactor ProgressRadial to use pseudo-elements #1356

Merged
merged 31 commits into from Jun 16, 2023
Merged

refactor ProgressRadial to use pseudo-elements #1356

merged 31 commits into from Jun 16, 2023

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Jun 14, 2023

Changes

Instead of using an svg, the radial progress indicator will now use pseudo-elements with a combination of conic and linear gradients. Based on this example: https://css-tricks.com/single-element-loaders-the-spinner/#aa-more-loader-examples

  • Both the HTML and CSS is greatly simplified.
  • The size/status is set using data attributes.
  • For the determinate variant, the percentage value will be set using a CSS variable --iui-progress-percentage.

Testing

Html preview: https://itwin.github.io/iTwinUI/1356/css/progress-indicator.html

Screen.Recording.2023-06-14.at.5.36.12.PM.mov

Storybook preview: https://itwin.github.io/iTwinUI/1356/react/?path=/story/progressindicators-progressradial--indeterminate

Screen.Recording.2023-06-14.at.5.03.33.PM.mov

Unit/visual tests updated.

Docs

Changeset added.

Briefly mentioned in migration guide: https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v3-migration-guide#progressradial--progresslinear

@mayank99 mayank99 self-assigned this Jun 14, 2023
@mayank99 mayank99 added this to the React 3.0 milestone Jun 14, 2023
@mayank99 mayank99 changed the title refactor ProgressRadial refactor ProgressRadial to use pseudo-elements Jun 15, 2023
}
&::after {
mask-image: conic-gradient(#0000, #000), linear-gradient(#000 0 0);
animation: rotate-indeterminate 0.8s linear infinite;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might want to use a custom cubic-bezier easing instead of linear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, because this is a gradient now, i'm not exactly sure how to handle forced-colors mode 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this slight ease-in-out cubic-bezier: https://cubic-bezier.com/#.6,.4,.4,.6

Copy link
Contributor

Choose a reason for hiding this comment

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

And yes, forced-colors mode is a bit strange. It doesn't work at all for linnear
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the easing

Screen.Recording.2023-06-16.at.9.15.39.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think i fixed forced-colors. have a look

@mayank99 mayank99 marked this pull request as ready for review June 15, 2023 13:46
@mayank99 mayank99 requested review from a team as code owners June 15, 2023 13:46
@mayank99 mayank99 requested review from gretanausedaite and removed request for a team June 15, 2023 13:46
@mayank99 mayank99 requested review from adamhock and FlyersPh9 and removed request for a team June 15, 2023 13:46
@FlyersPh9
Copy link
Collaborator

Once the indeterminate radial progress indicator completes or hits an error, it probably should not animate anymore.
Screenshot 2023-06-15 at 12 48 09 PM

I would expect something more like this:
Screenshot 2023-06-15 at 12 49 25 PM

@FlyersPh9
Copy link
Collaborator

Probably does not belong in this PR, but this example: should the spinner itself be a button to pause the upload? Or nest a borderless button within the spinner?
Screenshot 2023-06-15 at 12 50 27 PM

@mayank99
Copy link
Contributor Author

Once the indeterminate radial progress indicator completes or hits an error, it probably should not animate anymore.
Screenshot 2023-06-15 at 12 48 09 PM

I would expect something more like this:
Screenshot 2023-06-15 at 12 49 25 PM

Shouldn't that be left up to the user though?

e.g. Start with <ProgressRadial indeterminate /> then change to <ProgressRadial value={100} status='error' /> once it's done.


Probably does not belong in this PR, but this example: should the spinner itself be a button to pause the upload? Or nest a borderless button within the spinner?
Screenshot 2023-06-15 at 12 50 27 PM

The user can pass anything inside, so this should be possible:

<ProgressRadial>
  <IconButton styleType='borderless' size='small' aria-label='Pause'>
    <SvgPause />
  </IconButton>
</ProgressRadial>

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

LGTM (except force colors mode)

.changeset/quick-balloons-drum.md Outdated Show resolved Hide resolved
packages/itwinui-css/src/progress-indicator/radial.scss Outdated Show resolved Hide resolved
}
&::after {
mask-image: conic-gradient(#0000, #000), linear-gradient(#000 0 0);
animation: rotate-indeterminate 0.8s linear infinite;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this slight ease-in-out cubic-bezier: https://cubic-bezier.com/#.6,.4,.4,.6

Copy link
Contributor

@adamhock adamhock left a comment

Choose a reason for hiding this comment

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

Neat change!

So 25% is stroke-dashoffset: 66; or stroke-dashoffset: calc(88 - 88
* 25 / 100);
For determinate variants, percentage is set by
<code>--iui-progress-percentage</code>.
</p>
<br />

<div id="radial-size-large">
<!-- Small determinate -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't necessarily relate to your changes, but this might be a good time to change the comments from "Small" to "Large" for all of the large determinate examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mayank99 mayank99 merged commit 4c40085 into dev Jun 16, 2023
9 checks passed
@mayank99 mayank99 deleted the mayank/spinner branch June 16, 2023 20:07
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants