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)!: convert value range to 0-100 #10622

Merged

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Oct 25, 2024

Related Issue: #7207

Summary

Fixes the discrepancy in value range used between Progress (0 -> 1.0) and Loader (0 -> 100), to have components use a similar expected value.

BREAKING CHANGE: Refactors progress to use the value range of 0-100. Developers will need to provide a function to map existing values or use the updated range.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 25, 2024
Copy link
Contributor

@aPreciado88 aPreciado88 left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

The value being between 0 and 1 comes from the native progress:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress#value

Plus this would be a breaking change in my opinion. If we want consistency, maybe we should change loader instead, which doesn't have a native equivalent.

@Elijbet Elijbet marked this pull request as draft October 28, 2024 16:49
@Elijbet Elijbet changed the title fix(progress): converts value range to 0-100 fix(progress)!: converts value range to 0-100 Nov 1, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 15, 2024
@Elijbet Elijbet force-pushed the elijbet/7207-loader-progress-make-value-property-consistent branch from 7b111fb to b335773 Compare November 15, 2024 23:08
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Nov 19, 2024
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 19, 2024
@Elijbet Elijbet marked this pull request as ready for review November 19, 2024 17:56
@Elijbet Elijbet requested a review from jcfranco as a code owner November 19, 2024 17:56
@Elijbet Elijbet marked this pull request as draft November 19, 2024 18:03
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 19, 2024
@Elijbet Elijbet marked this pull request as ready for review November 19, 2024 18:34
@Elijbet
Copy link
Contributor Author

Elijbet commented Nov 19, 2024

The value being between 0 and 1 comes from the native progress: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress#value

Plus this would be a breaking change in my opinion. If we want consistency, maybe we should change loader instead, which doesn't have a native equivalent.

We decided not to go with 0-1 because of this little study: #7207 (comment)

Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Please roll back changes unrelated to the progress component.

@jcfranco jcfranco changed the title fix(progress)!: converts value range to 0-100 fix(progress)!: convert value range to 0-100 Nov 20, 2024
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

✨🚀✨

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

👍

@Elijbet Elijbet added skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 20, 2024
@Elijbet Elijbet merged commit f7d1d57 into dev Nov 20, 2024
13 checks passed
@Elijbet Elijbet deleted the elijbet/7207-loader-progress-make-value-property-consistent branch November 20, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants