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): percentage calculation #939

Merged
merged 1 commit into from
Nov 11, 2023
Merged

fix(Progress): percentage calculation #939

merged 1 commit into from
Nov 11, 2023

Conversation

maxsteinwand
Copy link
Contributor

@maxsteinwand maxsteinwand commented Nov 10, 2023

πŸ”— Linked issue

Fixes #938

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When the value exceeds the custom max value percent will be 100.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@benjamincanac
Copy link
Member

@DarkGhostHunter WDYT?

@benjamincanac benjamincanac changed the title <fix>[UProgress]: percentage calculation fix(Progress): percentage calculation Nov 10, 2023
Copy link

vercel bot commented Nov 10, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Nov 10, 2023 9:59am

@DarkGhostHunter
Copy link
Contributor

WDYT

Visually, the progress bar caps at 100%, so the percent should also be capped at that too. Since there is no sense in having "101% progress", this fix is correct. πŸ‘

The problem comes when the dev expects a value over 100% in the slot, which currently does. In this case it should be added to the docs that the percent value is capped to 100, instead of exceeding that.

@benjamincanac benjamincanac merged commit c55871b into nuxt:dev Nov 11, 2023
2 checks passed
@moshetanzer
Copy link
Collaborator

moshetanzer commented Jan 17, 2024

@maxsteinwand @DarkGhostHunter

Hi Hope you are well.

Think this should be partially reverted to show values over 100, but keep the default at 100, any reason not? If you want to track a workout and the user does more than required wouldn't it be correct that bar is full but user did more than 100% of the workout meant for them? Additionally, it seems strange to alter with the user set value?? Additionally in docs still is a "max" attribute but doesn't seem to do anything?

Struggling to understand the logic of this 🀷

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Jan 18, 2024

Think this should be partially reverted to show values over 100

Nope.

any reason not

Is a moot point since the developer already has access to the real value on the parent.

If you want to track a workout and the user does more than required wouldn't it be correct that bar is full but user did more than 100% of the workout meant for them?

You can put that value outside the progress bar itself. The inner value represents the progress of the bar, not the progress of the task, like "246".

Additionally, it seems strange to alter with the user set value??

It does not alter it. I only limits it inside the context of the progress bar.

Additionally in docs still is a "max" attribute but doesn't seem to do anything?

It does. It makes the 100% relative to that maximum number. For example, if you have 5 tasks, you set the maximum value to "5", hence the 100%. For each task done, the progress bar will be filled 20%. If you don't do that, then the progress bar would never be complete since each task would be 1%.

This kind of questions means that the docs need a little refactor to be more clear.

@maxsteinwand maxsteinwand deleted the fix/ProgressIndicator branch January 18, 2024 09:06
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.

[UProgress] Percentage not calculated correctly with max value
4 participants