Semantic-UI-React icon indicating copy to clipboard operation
Semantic-UI-React copied to clipboard

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

Open kangaechigai opened this issue 3 years ago • 2 comments

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

kangaechigai avatar Feb 03 '22 07:02 kangaechigai

💖 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.

welcome[bot] avatar Feb 03 '22 07:02 welcome[bot]

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.

brianespinosa avatar Feb 04 '22 00:02 brianespinosa