-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[rfw] Material slider widget #6610
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
e7fe375
- Added Material Slider widget
uberchilly 70c0c4d
Updated CHANGELOG.md
uberchilly a0d3e44
Fixed tests for web
uberchilly e36ff8f
Fixed formatting and end of line in utils
uberchilly 4174533
Merge branch 'main' into material_slider
uberchilly 77f6303
Merge branch 'main' into material_slider
uberchilly d502d46
- Fixed material slider handler indentations per guideline.
uberchilly 771f595
- Fixed unused import
uberchilly 6f89100
- Fixed formatting for utils
uberchilly 5b48d90
Merge branch 'main' into material_slider
uberchilly 905f8a9
Merge branch 'main' into material_slider
uberchilly 77028ca
Merge branch 'main' into material_slider
uberchilly bb567e6
Merge branch 'main' into material_slider
stuartmorgan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the number of decimals be configurable from the definition as well? I think having a "0 decimals" which calls
round
might be a very common use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw technically speaking, since we cannot really "script" from outside easily having label as string param is not useful since one can only provide hardcoded text that is why I've also added common use case of showing current value with 2 decimal places but I can also add number of decimal places as a param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uberchilly I agree. I also think the current solution is not great for i18n. This maybe could be re-implemented with something similar to
sprintf
or a way for users to specify where in the string they want the number (to enable them to pass'## Birds'
so the label is "18 Birds" rather than "Birds: 18") (but I don't think this is a blocker for the feature).I don't think 2 decimal places is more common than 1 decimal place or 0 (or 5), it all depends on the number of "divisions" or the size of the increment. For example, if I were to use this widget with integer values (between 0 and 100 with a step of 10), I think I wouldn't want to see any decimal positions anywhere.
If I was doing between 0 to 1 with 1000 divisions, I'd certainly would want to see increments in the 3rd decimal.
Instead of attempting to figure out what's the ideal number of decimals, I'd just add a value so users can configure it (and that's why I suggested it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in general, but on the other hand other already supported widgets have bigger limitations imo. In my personal project because of these limitations I had to also add DoubleText and IntText as local widgets because I couldn't do what was suggested here somewhere and that was to add string variant of value that I need to show in Text in data beside regular double for example, and showing number value in Text widget is much more common usecase than using label in slider. So, I am not sure how deep should each widget go in terms of supporting things vs an effort to add scripting easily to prepare values from outside