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

feat: add Circular Progress #2122

Merged
merged 11 commits into from
Apr 18, 2024
Merged

feat: add Circular Progress #2122

merged 11 commits into from
Apr 18, 2024

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Apr 8, 2024

API

Current API

  • <ProgressBar variant="meter | progress">

Ideal New API

  • <ProgressBar type="meter | progress" variant="linear | circular" >

Proposed API to Avoid Breaking Change

  • <ProgressBar type="meter | progress" variant="linear | circular | meter | progress">
  • If type prop is provided throw an error when the variant is meter or progress.
  • Provide a codemod to migrate to the new API.
  • Eventually, remove support for the old API & move to the Ideal API in the next major release.

Related Stacked PRs

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Copy link

changeset-bot bot commented Apr 8, 2024

🦋 Changeset detected

Latest commit: 9e20581

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Apr 8, 2024

✅ PR title follows Conventional Commits specification.

@snitin315 snitin315 force-pushed the feat/circular-progressbar branch from db5a314 to 3109768 Compare April 15, 2024 11:11
Copy link

codesandbox-ci bot commented Apr 15, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9e20581:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Apr 15, 2024

Bundle Size Report

Updated Components
Status Component Base Size (kb) Current Size (kb) Diff
FileUpload 13.476 14.561 +1.085 KB
ProgressBar 1.701 3.094 +1.393 KB

Generated by 🚫 dangerJS against 9e20581

@anuraghazra
Copy link
Member

@snitin315 can you check this once? this is also present on master. Not sure if this layout shift is intended or not.

Screen.Recording.2024-04-16.at.4.23.02.PM.mov

if (progressVariant === 'circular' && isIndeterminate) {
throwBladeError({
moduleName: 'ProgressBar',
message: `Cannot set 'isIndeterminate' when 'variant' is 'circular'.`,
Copy link
Member

Choose a reason for hiding this comment

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

So we aren't sufficing the spinner usecase with Circular progressbar is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The Spinner will be a separate component.

Comment on lines 20 to 21
size: 24,
strokeWidth: 3,
Copy link
Member

Choose a reason for hiding this comment

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

these values are pixels or spacing tokens or sizing tokens?

Copy link
Member Author

Choose a reason for hiding this comment

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

size tokens, updated 👍🏻

Copy link
Member

@anuraghazra anuraghazra left a comment

Choose a reason for hiding this comment

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

Mostly lgtm, minor comments.

@snitin315
Copy link
Member Author

@snitin315 can you check this once? this is also present on master. Not sure if this layout shift is intended or not.

@anuraghazra I'll check this separately.

Copy link
Member

Choose a reason for hiding this comment

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

https://61c19ee8d3d282003ac1d81c-qbudppbfos.chromatic.com/?path=/story/components-progressbar--circular-progress&args=value:36;type:meter;isIndeterminate:!false;showPercentage:!true

The percentage is not showing up even with the showPercentage: true option in meter. Is that expected? There might be usecases of showing percentage in middle in circular meter on dashboard no?

Copy link
Member Author

@snitin315 snitin315 Apr 18, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Lets remove the variant meter and progress from storybooks table?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's using autodocs so it's auto-generated, we will need to manually define all props otherwise. I think it's fine to keep it as is now since it is supported.

anuraghazra
anuraghazra previously approved these changes Apr 18, 2024
@snitin315 snitin315 merged commit 410cfb5 into master Apr 18, 2024
17 checks passed
@snitin315 snitin315 deleted the feat/circular-progressbar branch April 18, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L1 First level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants