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

Add Progress and ProgressBar components #39

Merged
merged 7 commits into from
Dec 10, 2023
Merged

Conversation

dricair
Copy link
Contributor

@dricair dricair commented Nov 25, 2023

Copy link
Owner

@isosphere isosphere left a comment

Choose a reason for hiding this comment

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

Thanks for another contribution!

While I agree with error checking, I don't love being a library that panic!s when bounds are exceeded. This could conceivably happen if the value is bound and modified to be out of bounds. I think I'd rather we clamp the value to be max if it's above max and ditto for min and complain with a console error somewhere. The entire Yew application we provide components for shouldn't crash for this kind of violation.

@dricair
Copy link
Contributor Author

dricair commented Dec 1, 2023

Ok I think you're right. I tried to not change the values as much as possible except to avoid the div to panic.
Note that warn does not print anything, I think we should replace by gloo::console instead to display on the browser console.

@isosphere
Copy link
Owner

I think we should replace by gloo::console instead to display on the browser console.

I agree! That's a pretty common dependency, I think it's reasonable for us to depend on it so we can provide better debugging info.

@dricair
Copy link
Contributor Author

dricair commented Dec 2, 2023

I added a commit to change log::warn to gloo_console::warn.
It is almost directly compatible except that later do not do format! itself.

@isosphere isosphere merged commit 110a248 into isosphere:main Dec 10, 2023
1 check passed
@isosphere
Copy link
Owner

Thanks for the adjustment; merged with main as 0.9.0.

I feel the bump in minor version coveys the new feature supported better.

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.

2 participants