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

Number input widget #762

Closed
wants to merge 12 commits into from
Closed

Number input widget #762

wants to merge 12 commits into from

Conversation

hangleang
Copy link

Hi there, I would like to PR that implement number input widget in #209 issue. I have also added a simple example for show case this widget. I'm look forward to see any idea, feel free to improve it or leave a comment here.

@13r0ck
Copy link
Contributor

13r0ck commented Mar 5, 2021

It looks good so far, there are some comments about how to remove the Self from your messages. I really wish that it accepted scroll inputs to increase and decrease the values, as well if I hold on the up/down arrow it would make sense for the value to increase while held.

There is a bug with your implementation of the max value. Lets say I clear the input to 0 then i put my cursor infront of the |0 (like that) then i press, for example 9, then the input is 9|0 with my cursor in the middle. Though if I press 9 again the input is blocked rather than overwriting the 0. If would feel more natural to go from 9|0 -> 99| which is still under 255 (the max in your example), though your code checks the input as 99|0 which is above 255 and blocks the input. Hopefully I makes sense?

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Thank you so much for this! 😄

It may be interesting to contribute this widget to iced_aw first. This is, since very recently, the official community crate for additional widgets (see #761). I think it will be easier for users to contribute and improve the widget further there and then, once it eventually matures enough, we could migrate it to this repository.

Ideally, we should leave this repository for important fixes and core changes. This way, I can stop bottlenecking interesting contributions while focusing on the core parts of the library!

@Kaiden42
Copy link
Contributor

Kaiden42 commented Mar 6, 2021

It may be interesting to contribute this widget to iced_aw first.

Sounds like a good idea 😉. If you have problems with moving your widget to iced_aw, please let me know.

I really wish that it accepted scroll inputs to increase and decrease the values,

This can be easily done with mouse::Event::WheelScrolled { delta }. You can take a look at the Color Picker implementation for this.

as well if I hold on the up/down arrow it would make sense for the value to increase while held.

As far as I know, this is currently not possible. It would require the widget to send an update/message by itself after some period, wich is currently not implemented. This is slightly related to #31 and #560 .

There is a bug with your implementation of the max value. Lets say I clear the input to 0 then i put my cursor infront of the |0 (like that) then i press, for example 9, then the input is 9|0 with my cursor in the middle. Though if I press 9 again the input is blocked rather than overwriting the 0. If would feel more natural to go from 9|0 -> 99| which is still under 255 (the max in your example), though your code checks the input as 99|0 which is above 255 and blocks the input. Hopefully I makes sense?

For comparison, I looked at how the number-input field of HTML handles this (chromium). The maximum value there, is not considered at all with the text input. With a max value of 220 you can easily enter 9999990 or similar. I find persöhnlich also not just intuitive.

However, with your suggestion I would rather go for 99|0 -> 255 instead of 99|0 -> 99. Maybe we can look at how other GUI libraries handle this case and pick out the best behavior for us.

@hangleang
Copy link
Author

It looks good so far, there are some comments about how to remove the Self from your messages. I really wish that it accepted scroll inputs to increase and decrease the values, as well if I hold on the up/down arrow it would make sense for the value to increase while held.

Thank for review @13r0ck. I've format the example for convenient as you told. I also merge with your branch(more_scroll) and in the integration, I've change on scroll event to increase or decrease the value directly inside widget logic because I don't want user to handle those two messages by themselves, but the logic its self is still the same. I was given you credit on that section, Hope you don't mind on that.

There is a bug with your implementation of the max value. Lets say I clear the input to 0 then i put my cursor infront of the |0 (like that) then i press, for example 9, then the input is 9|0 with my cursor in the middle. Though if I press 9 again the input is blocked rather than overwriting the 0. If would feel more natural to go from 9|0 -> 99| which is still under 255 (the max in your example), though your code checks the input as 99|0 which is above 255 and blocks the input. Hopefully I makes sense?

This update is fixed those bugs if I do understand you well. So, please check on that again if there is some point I missing. Leave it a comment. Thank again for spending your time on review.

@hangleang
Copy link
Author

Thank you so much for this! smile
It may be interesting to contribute this widget to iced_aw first. This is, since very recently, the official community crate for additional widgets (see #761). I think it will be easier for users to contribute and improve the widget further there and then, once it eventually matures enough, we could migrate it to this repository.

Yeah, you are welcome. I'll put this widget in that repo when the time allowed. I got a lot of ideas from the repo, hahaa.

Ideally, we should leave this repository for important fixes and core changes. This way, I can stop bottlenecking interesting contributions while focusing on the core parts of the library!

Ohh I see. Thank for warm welcome.

@hangleang
Copy link
Author

It may be interesting to contribute this widget to iced_aw first.

Thank for interested on the widget. Good if we can works together.

Sounds like a good idea wink. If you have problems with moving your widget to iced_aw, please let me know.

Not now, but soon hahaa.

I really wish that it accepted scroll inputs to increase and decrease the values,
This can be easily done with mouse::Event::WheelScrolled { delta }. You can take a look at the Color Picker implementation for this.

This update has implementation on that event which I merge from @13r0ck. Thank to @13r0ck.

There is a bug with your implementation of the max value. Lets say I clear the input to 0 then i put my cursor infront of the |0 (like that) then i press, for example 9, then the input is 9|0 with my cursor in the middle. Though if I press 9 again the input is blocked rather than overwriting the 0. If would feel more natural to go from 9|0 -> 99| which is still under 255 (the max in your example), though your code checks the input as 99|0 which is above 255 and blocks the input. Hopefully I makes sense?

For comparison, I looked at how the number-input field of HTML handles this (chromium). The maximum value there, is not considered at all with the text input. With a max value of 220 you can easily enter 9999990 or similar. I find persöhnlich also not just intuitive.

However, with your suggestion I would rather go for 99|0 -> 255 instead of 99|0 -> 99. Maybe we can look at how other GUI libraries handle this case and pick out the best behavior for us.

This widget is required max value for checking valid number, not like the number input in HTML.

@13r0ck
Copy link
Contributor

13r0ck commented Mar 8, 2021

I am going to hold out on more input until this PR is moved over to Iced aw. This is going to be a useful widget!

@13r0ck 13r0ck mentioned this pull request May 6, 2021
@Kaiden42
Copy link
Contributor

Kaiden42 commented May 6, 2021

Since this is now merged into iced_aw this PR can be closed now. 🙂

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.

4 participants