Don't rebuild tooltips unnecessarily with updateOptions

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.7k stars 657 forks source link

Don't rebuild tooltips unnecessarily with updateOptions #1184

Closed eoghanmurray closed 2 years ago

eoghanmurray commented 2 years ago

problem manifested when using example recipe 'Merging overlapping tooltips' and then subsequently doing updateOptions({margin: 60})
problem was that tooltips no longer exhibeted overlapping behaviour as they had been recreated from original config

ref: https://refreshless.com/nouislider/examples/#section-merging-tooltips

eoghanmurray commented 2 years ago

I wonder whether options.pips should be given the same treatment; in case there's been any customisation to the pips outside of the knowledge of the slider? Update: have made that change in next commit

leongersen commented 2 years ago

Thanks for submitting this PR. I get the reason for this change, but making this change would mean the tooltips and pips would no longer be updated to match other updated properties.

I'd recommend that you call mergeTooltips (from the example) again after updateOptions.

eoghanmurray commented 2 years ago

making this change would mean the tooltips and pips would no longer be updated to match other updated properties

Did I break something? The intention was to maintain all existing methods of updating?

leongersen commented 2 years ago

With these changes, updating range is no longer reflected in the new pips.

eoghanmurray commented 2 years ago

Would it be a good idea to build up a list of dependencies for the tooltips? E.g. if margin changes don't update the tooltips, whereas do so if range is passed in?