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

fix(Progress): clamp bar width and improve propTypes check for progress prop #4332

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kangaechigai
Copy link

Fix some inconsistencies in Progress bar width and invalid prop combos.

  1. bar width: progress='value' and value > total
    e.g. <Progress progress='value' value={51} total={50} />
     
    Expected Result: progress bar with text 51 and width clamped to 100% to match behavior with any other value for progress
     
    Actual Result: progress bar with text 51 and width 102%

  2. bar width: value without total
    e.g. <Progress value={50} />
     
    Expected Result: progress bar with width 50% to match behavior with progress='value'
     
    Actual Result: progress bar with min width

  3. bad prop combo: progress='ratio without value or total
    e.g. <Progress progress="ratio" />
     
    Expected Result: propTypes error
     
    Actual Result: no propTypes error, progress bar with text undefined/undefined

  4. bad prop combo: progress='value' without value
    e.g. <Progress progress="value" />
     
    Expected Result: propTypes error
     
    Actual Result: no propTypes error

Version

2.1.1

Testcase

https://codesandbox.io/s/progress-problem-with-progress-value-odipb?file=/index.js

- calculatePercent handles all valid cases
- getPercent just clamps and rounds output of calculatePercent
- don't allow progress='value' without value
- don't allow progress='ratio' without value and total
@welcome
Copy link

welcome bot commented Feb 3, 2022

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@brianespinosa
Copy link
Member

I thought for sure that the progress bar would allow to go past 100 in the parent SUI project, but I wanted to verify. Sure enough, it does not let you go past 100%. I put a Codesandbox together to check just to make sure, as that parent project is our source of truth. We get an error if the percentage exceeds 100%. Looks like this was something that was missed when the Progress component was built. But maybe this was intentional.

This could be considered a breaking change for this component as some people might be using it to show values exceeding 100%. I know there are scenarios in some of the projects I use the React bindings where we have used the Progress component in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants