additional class support on 'rangeHighlights' option #742
Conversation
|
It looks like we need a unit test to make sure this works correctly. After that, I think it looks good. |
|
Yup, unit test are updated as well. |
|
we'd prefer if it wasn't there, yes. |
|
@jccode the dist directory is compiled code, which is generated from the code in the src directory. Please make your changes to the src directory code thanks! |
|
@rovolution I know. My code changes are already apply to src directory. |
|
Just don't commit |
|
@seiyria I rollback the |
There was a problem hiding this comment.
@jccode looks solid! I left some brief feedback but once thats incorporated ping me for a final review
src/js/bootstrap-slider.js
Outdated
| if (Array.isArray(this.options.rangeHighlights) && this.options.rangeHighlights.length > 0) { | ||
| for (let j = 0; j < this.options.rangeHighlights.length; j++) { | ||
|
|
||
| var rangeHighlightsOpts = this.options.rangeHighlights; |
There was a problem hiding this comment.
could you fix the spacing and declare this as a const instead of a var since the value is not being reassigned? thanks!
|
Sorry that I wan't aware of the spacing issue. Thanks for your review. |
There was a problem hiding this comment.
@jccode sorry! one more piece of feedback and it should be good.
I pulled it down and tested it locally and it works great btw...awesome work!
src/js/bootstrap-slider.js
Outdated
| var rangeHighlightElement = document.createElement("div"); | ||
| rangeHighlightElement.className = "slider-rangeHighlight slider-selection"; | ||
|
|
||
| rangeHighlightElement.className = "slider-rangeHighlight slider-selection " + (rangeHighlightsOpts[j].class || ""); |
There was a problem hiding this comment.
could you refactor this to use an ES6 template string instead? I think it will help make the code a bit cleaner:
const customClassString = rangeHighlightsOpts[j].class || "";
rangeHighlightElement.className = `slider-rangeHighlight slider-selection ${customClassString}`;There was a problem hiding this comment.
WOW! Thanks for the great suggestion.
|
@jccode merged and published to 9.8.0. Thanks again for your contribution! |
Hi seiyria, would you please accept the pull request? Thank you.