Add 'step' as a configuration attribute in Slider #1500

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@customcommander
Contributor

customcommander commented Dec 15, 2013

Hi,

This is an attempt to implement #1449 (provided that I correctly understood the requirement).

Pinging owner @lsmith

Cheers,

+ step: {
+ value: 1,
+ validator: function (value) {
+ return Y.Lang.isNumber(value) && value > 0 && value <= this.get('max');

This comment has been minimized.

@lsmith

lsmith Dec 16, 2013

Contributor

min can be greater than max for value-reversed Sliders.

@lsmith

lsmith Dec 16, 2013

Contributor

min can be greater than max for value-reversed Sliders.

This comment has been minimized.

@customcommander

customcommander Dec 16, 2013

Contributor

Ouch, I didn't know those existed.

@customcommander

customcommander Dec 16, 2013

Contributor

Ouch, I didn't know those existed.

+ value: 10,
+ getter: function (value) {
+ var step = this.get('step');
+ return (step==1) ? value : step+value;

This comment has been minimized.

@lsmith

lsmith Dec 16, 2013

Contributor

This kind of hard coding isn't very library friendly. Why the special casing of 1? Also, == should be === unless coercion is needed.

@lsmith

lsmith Dec 16, 2013

Contributor

This kind of hard coding isn't very library friendly. Why the special casing of 1? Also, == should be === unless coercion is needed.

This comment has been minimized.

@customcommander

customcommander Dec 16, 2013

Contributor

Which hard coding? value: 10 was there before. Or is the getter the offending code?

About the special casing of 1, I wanted to avoid (given minor and major steps are both set to 1 and 10):

value after minor inc after major inc
1 3 14

Which was failing the existing tests. As opposed to:

value after minor inc after major inc
1 2 12

Which I thought was the expected behavior.

I didn't bother to === check again since I made sure that step is a number through its validator function. But I don't mind changing this.

@customcommander

customcommander Dec 16, 2013

Contributor

Which hard coding? value: 10 was there before. Or is the getter the offending code?

About the special casing of 1, I wanted to avoid (given minor and major steps are both set to 1 and 10):

value after minor inc after major inc
1 3 14

Which was failing the existing tests. As opposed to:

value after minor inc after major inc
1 2 12

Which I thought was the expected behavior.

I didn't bother to === check again since I made sure that step is a number through its validator function. But I don't mind changing this.

This comment has been minimized.

@lsmith

lsmith Dec 16, 2013

Contributor

value: 10 is configurable, but having 1 hard coded in the logic of the getter is what makes it difficult for implementers to customize. It's easier to customize by configuration than to override, and harder still to override attribute getter (or any function). In general, it's preferable to use string values as getter/setter/validator/valueFn configurations because they use late binding to resolve the method, so is easier for subclassing or class extension overrides. FYI.

@lsmith

lsmith Dec 16, 2013

Contributor

value: 10 is configurable, but having 1 hard coded in the logic of the getter is what makes it difficult for implementers to customize. It's easier to customize by configuration than to override, and harder still to override attribute getter (or any function). In general, it's preferable to use string values as getter/setter/validator/valueFn configurations because they use late binding to resolve the method, so is easier for subclassing or class extension overrides. FYI.

This comment has been minimized.

@customcommander

customcommander Dec 17, 2013

Contributor

Ah yeah that makes sense

@customcommander

customcommander Dec 17, 2013

Contributor

Ah yeah that makes sense

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Dec 16, 2013

Contributor

I'm not sure I understand the purpose of this PR. Slider already supports minorStep and majorStep for keyboard interaction, and I don't see any change to the parts of the code that deal with mouse events. What does this add?

Contributor

lsmith commented Dec 16, 2013

I'm not sure I understand the purpose of this PR. Slider already supports minorStep and majorStep for keyboard interaction, and I don't see any change to the parts of the code that deal with mouse events. What does this add?

@customcommander

This comment has been minimized.

Show comment
Hide comment
@customcommander

customcommander Dec 16, 2013

Contributor

As mentioned above I wasn't sure if I had fully understood the requirement.

My attempt was simply to modify the behavior of minor and major steps as I assumed these were picked up internally to increment/decrement the value of the Slider.

So I just assumed that it would work out of the box seeing that the keyboard automation was passing. I did forget about the mouse indeed.

Obviously this PR has failed at so many levels so quite happy to abandon it.

Thanks

Contributor

customcommander commented Dec 16, 2013

As mentioned above I wasn't sure if I had fully understood the requirement.

My attempt was simply to modify the behavior of minor and major steps as I assumed these were picked up internally to increment/decrement the value of the Slider.

So I just assumed that it would work out of the box seeing that the keyboard automation was passing. I did forget about the mouse indeed.

Obviously this PR has failed at so many levels so quite happy to abandon it.

Thanks

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Dec 16, 2013

Contributor

If you do want to dedicate the time to add support for value ticks (e.g. 0, 10, 20,...), it would make a lot of people happy! :)

Contributor

lsmith commented Dec 16, 2013

If you do want to dedicate the time to add support for value ticks (e.g. 0, 10, 20,...), it would make a lot of people happy! :)

@lsmith lsmith closed this Dec 16, 2013

@customcommander customcommander deleted the customcommander:slider-step branch Dec 16, 2013

@customcommander

This comment has been minimized.

Show comment
Hide comment
@customcommander

customcommander Dec 17, 2013

Contributor

Sorry but I'm a bit confused as to what the difference is between the two features: step and tick? I'd like to give it another shot but I could use a bit of clarification to be honest.

Contributor

customcommander commented Dec 17, 2013

Sorry but I'm a bit confused as to what the difference is between the two features: step and tick? I'd like to give it another shot but I could use a bit of clarification to be honest.

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Dec 17, 2013

Contributor

Sorry for the confusion. Ticks are discrete values that the slider thumb would jump between as the user interacted with the slider either by keyboard or mouse. If you look at the "Slider with Steps and Snapping" example of this Slider (randomly googled) http://code.ovidiu.ch/slider/, hopefully you'll see what I mean.

Rather than minorStep and majorStep which operate only via keyboard and don't limit the available values--only increment or decrement the value by the step amount--ticks disallow all but a whitelisted set of values.

Contributor

lsmith commented Dec 17, 2013

Sorry for the confusion. Ticks are discrete values that the slider thumb would jump between as the user interacted with the slider either by keyboard or mouse. If you look at the "Slider with Steps and Snapping" example of this Slider (randomly googled) http://code.ovidiu.ch/slider/, hopefully you'll see what I mean.

Rather than minorStep and majorStep which operate only via keyboard and don't limit the available values--only increment or decrement the value by the step amount--ticks disallow all but a whitelisted set of values.

@customcommander

This comment has been minimized.

Show comment
Hide comment
@customcommander

customcommander Dec 17, 2013

Contributor

I think I got it now :)

Contributor

customcommander commented Dec 17, 2013

I think I got it now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment