-
Notifications
You must be signed in to change notification settings - Fork 0
Reintroducing FloatSlider from legacy with an example. #226
Conversation
#: is restricted to 65535, while the default is 100. | ||
precision = Range(value=100, low=1, high=MAX_SLIDER_VALUE) | ||
|
||
#: Defines the number of steps that the slider will move when the |
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 do not think that single_step and page_step need to be redefined.
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.
They are redefined to put the upper bound at 'precision'.
Any other feedback? It would be convenient to have this in master. |
Im on holiday and haven't had a chance to look yet.
|
Sorry about that. No problem. Happy new year! |
I'm wondering if it would just be easier to have an adapter bolted onto a Slider, rather than do the mapping on the backend... but maybe not, i'm not sure yet. I don't like that we have to duplicate so much code (like the property getters and setters) but that may just be a traits-ism. |
#: user presses the page_up/page_down keys. The Default is 10. An | ||
#: upper limit may be imposed on this value according to the limits | ||
#: of the client widget. | ||
page_step = Range(low='_one', high='precision', value=10) |
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'm not sure about clipping to 'precision'. This implies that we will update page_step
if precision
becomes too small. However, the Range
trait does not do any dependency analysis or notification, so the user would still get in trouble if they set the step then changed the precision. I'm not sure that protecting the user from themselves in this case is needed.
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.
Whatever you like.
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.
Surely you have an opinion :)
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 really don't. If you think the Range
is problematic, that's fine. Just get it done. It's unlikely anyone is going to change them in any case.
Putting the range transforms on the Slider would cut down on code duplication. It would also make it easier to build a logarithmic slider, for example. On the down side, this would complicate the Slider interface. |
Robert, what's your opinion of a generic slider transform, instead of a totally different slider? |
Didn't work very well, so I wrote the |
This is what I had in mind with regards to the transform: This works, and it should be simple enough for someone to write a new transform for various ranges or scales. Implementation: |
If you guys like this idea, I'll probably move the implementation into |
Sure. |
Chris F., what are your thoughts? |
This looks like a great solution to me. It complicates slightly what used to be Thanks. |
Closing in favor of #237 |
No description provided.