leongersen / noUiSlider

noUiSlider is a lightweight, ARIA-accessible JavaScript range slider with multi-touch and keyboard support. It is fully GPU animated: no reflows, so it is fast; even on older devices. It also fits wonderfully in responsive designs and has no dependencies.
https://refreshless.com/nouislider/
MIT License
5.69k stars 656 forks source link

Consider null values the same way as undefined (revert to defaults) #856

Closed webketje closed 6 years ago

webketje commented 6 years ago

Issue reproduction

Proposal Change options.js L433 options[name] === undefined to options[name] === undefined || options[name] === null (and all other options property checks where applicable*). Eventually noUiSlider could benefit from another utility like isNumeric:

function isUndef(v) {
  return v !== undefined && v !== null;
}

where applicable = I might've missed a detail in the source where null is expected to behave differently from undefined, please do point it out*. If I get enough time I'll reference this issue from a PR.

Motivation
'Modern' front-end frameworks often require one to specify component properties/state up front (for reactivity). Some properties may be assigned null, rather than undefined, as per the widespread convention stated in, e.g., the accepted SO answer on what is the difference between null and undefined in javascript. When passed to noUiSlider, properties that are either null or undefined ought to be considered in the same way.

Disclaimer I'm working on a (full) noUiSlider integration for Vue, and need to map parameters that are null, to undefined, for the slider to work as expected.

Thanks for reading

leongersen commented 6 years ago

Maybe I'm misunderstanding, but are you arguing that the plugin should throw the same exceptions for null and undefined? If so, I agree.

webketje commented 6 years ago

Well I'm argueing that null should be considered the same as undefined, so it's 2 things:
1 is: throw the same exceptions (as you confirmed);
2 is: change current behavior:
options[name] === undefined ? defaults[name] : options[name]
to
options[name] === (undefined || null) ? defaults[name] : options[name]

leongersen commented 6 years ago

Alright, cool. I'd be happy with a pull request (utility style) patching this. If you can, add a unit test or two to tests/slider_errors.js to show the unexpected behaviour.

The cssPrefix and cssClasses check for undefined themselves; they shouldn't. Instead, they should be marked required as a default is provided.

leongersen commented 6 years ago

I just released noUiSlider 11.1.0, which implements this. Thanks for contributing!

webketje commented 6 years ago

@leongersen Thanks, sorry I couldn't help as I didn't get the chance to spend enough time to make a good PR. Keep it up!

github-actions[bot] commented 2 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.