Feature : rtl mode (with options) #679

Shnoulle · 2016-12-30T11:05:31Z

Pull Requests

JSFiddle https://jsfiddle.net/tvf4gt4g/ + https://jsfiddle.net/t7ccex4a/1/
Github issue Add a general rtl mode ( !==inverse ) #678
documentation updates to README file
examples within /tpl/index.tpl
grunt test : Done, without errors.

Feedback

rewrite using ES6 template string
refactor imbricated ternary test
RtlOptionsSpec unit test

- [x] Add a rtl option (default false) - [x] Set inlined style according to this option - [x] Add slider-rtl class to wrapper ( slider (slider-horizontal|slider-vertical) - [x] Fix css file according top this class - [x] Set rtl mode automatic according to dir="rtl"

[x] documentation updates to README file [x] examples within [/tpl/index.tpl](https://github.com/seiyria/bootstrap-slider/blob/master/tpl/index.tpl)

seiyria · 2016-12-30T13:57:29Z

Definitely a good start, however:

It needs to pass our unit tests
You need to add some unit tests to this

Shnoulle · 2016-12-30T14:05:10Z

OK, units test passed (and errors fixed)

2 questions

About 'default' : rtl : bool|null : null (or not set) => default ?
Unit test : only some ? Add one rtl (forced) everywhere ?

seiyria · 2016-12-30T14:08:13Z

not set is a fine default since it's falsy. as for unit tests, you'd have to ask @rovolution specifically on what he'd like to see with unit tests.

Shnoulle · 2016-12-30T14:33:46Z

src/js/bootstrap-slider.js

@@ -384,16 +384,23 @@ const windowIsDefined = (typeof window === "object");
 				this.options[optName] = val;
 			}

+			// Check options.rtl
+			if(this.options.rtl==='auto'){


Alternative

if (this.options.rtl === null) { this.options.rtl = window.getComputedStyle(this.element).direction === 'rtl'; }

Shnoulle · 2016-12-30T14:34:18Z

src/js/bootstrap-slider.js

@@ -829,6 +848,7 @@ const windowIsDefined = (typeof window === "object");
 				tooltip_split: false,
 				handle: 'round',
 				reversed: false,
+				rtl: 'auto',


Alt : rtl: null,

Shnoulle · 2016-12-30T14:34:35Z

About 'default' : currently bool|string => string==='default' use dir='rtl'.

But null!==false see 6dadef3#r94231462

Else, bout unit test : 2 solutions : add some rtl=true in some of existing tests, or add a new global test RtlOptionsSpec. Wait for @rovolution :)

rovolution

@Shnoulle Thanks for taking time to submit a PR! It looks great for the most part! I left some specific feedback in the code, so if you could take some time to incorporate that in, it would be much appreciated!

1) I would like to see some unit tests for this new option to validate that it effects the slider in the desired way based upon what value it is set to.

2) Go ahead and create a new spec file (something like RtlOptionsSpec) to contain the tests for this particular feature.

rovolution · 2017-01-02T22:24:35Z

src/js/bootstrap-slider.js

@@ -1196,7 +1216,11 @@ const windowIsDefined = (typeof window === "object");
 								this.rangeHighlightElements[i].style.top = `${currentRange.start}%`;
 								this.rangeHighlightElements[i].style.height = `${currentRange.size}%`;
 							} else {
-								this.rangeHighlightElements[i].style.left = `${currentRange.start}%`;
+								if(this.options.rtl){
+									this.rangeHighlightElements[i].style.right = currentRange.start + "%";


could you rewrite this using an ES6 template string?

this.rangeHighlightElements[i].style.right = `${currentRange.start}%`;

rovolution · 2017-01-02T22:24:42Z

src/js/bootstrap-slider.js

+								if(this.options.rtl){
+									this.rangeHighlightElements[i].style.right = currentRange.start + "%";
+								} else {
+									this.rangeHighlightElements[i].style.left = currentRange.start + "%";


could you rewrite this using an ES6 template string?

rovolution · 2017-01-02T22:27:05Z

src/js/bootstrap-slider.js

@@ -1209,7 +1233,7 @@ const windowIsDefined = (typeof window === "object");
 				if (Array.isArray(this.options.ticks) && this.options.ticks.length > 0) {

 					var styleSize = this.options.orientation === 'vertical' ? 'height' : 'width';
-					var styleMargin = this.options.orientation === 'vertical' ? 'marginTop' : 'marginLeft';
+					var styleMargin = this.options.orientation === 'vertical' ? 'marginTop' : (this.options.rtl ? 'marginRight' : 'marginLeft');


instead of doing nested ternary (which is a little difficult to read), could you just re-write the assignment of this variable to use an if...else?

var styleMargin; if (this.options.orientation === 'vertical') { styleMargin = 'marginTop'; } else if (this.options.rtl) { styleMargin = 'marginRight'; } else { styleMargin = 'marginLeft'; }

rovolution · 2017-01-02T22:27:42Z

src/js/bootstrap-slider.js

-								this.tickLabels[i].style['marginLeft'] =  this.sliderElem.offsetWidth + 'px';
-								this.tickLabelContainer.style['marginTop'] = this.sliderElem.offsetWidth / 2 * -1 + 'px';
+								if(this.options.rtl){
+									this.tickLabels[i].style['marginRight'] = this.sliderElem.offsetWidth + 'px';


could you rewrite this using an ES6 template string?

rovolution · 2017-01-02T22:27:45Z

src/js/bootstrap-slider.js

+								if(this.options.rtl){
+									this.tickLabels[i].style['marginRight'] = this.sliderElem.offsetWidth + 'px';
+								}else{
+									this.tickLabels[i].style['marginLeft'] = this.sliderElem.offsetWidth + 'px';


could you rewrite this using an ES6 template string?

rovolution · 2017-01-02T22:30:42Z

src/js/bootstrap-slider.js

@@ -1299,15 +1327,15 @@ const windowIsDefined = (typeof window === "object");
 					if (this.options.orientation === 'vertical') {
 						this._css(this.tooltip_min, 'margin-top', -this.tooltip_min.offsetHeight / 2 + 'px');
 					} else {
-						this._css(this.tooltip_min, 'margin-left', -this.tooltip_min.offsetWidth / 2 + 'px');
+						this._css(this.tooltip_min, 'margin-'+this.stylePos, -this.tooltip_min.offsetWidth / 2 + 'px');


could you rewrite this using an ES6 template string?

`margin-${this.stylePos}`

rovolution · 2017-01-02T22:30:45Z

src/js/bootstrap-slider.js

 					}

 					if (this.options.orientation === 'vertical') {
 						this._css(this.tooltip, 'margin-top', -this.tooltip.offsetHeight / 2 + 'px');
 					} else {
-						this._css(this.tooltip, 'margin-left', -this.tooltip.offsetWidth / 2 + 'px');
+						this._css(this.tooltip, 'margin-'+this.stylePos, -this.tooltip.offsetWidth / 2 + 'px');


could you rewrite this using an ES6 template string?

`margin-${this.stylePos}`

rovolution · 2017-01-02T22:30:49Z

src/js/bootstrap-slider.js

@@ -1279,13 +1307,13 @@ const windowIsDefined = (typeof window === "object");
 					if (this.options.orientation === 'vertical') {
 						this._css(this.tooltip, 'margin-top', -this.tooltip.offsetHeight / 2 + 'px');
 					} else {
-						this._css(this.tooltip, 'margin-left', -this.tooltip.offsetWidth / 2 + 'px');
+						this._css(this.tooltip, 'margin-'+this.stylePos, -this.tooltip.offsetWidth / 2 + 'px');


could you rewrite this using an ES6 template string?

`margin-${this.stylePos}`

rovolution · 2017-01-02T22:30:57Z

src/js/bootstrap-slider.js

@@ -1317,7 +1345,7 @@ const windowIsDefined = (typeof window === "object");
 					if (this.options.orientation === 'vertical') {
 						this._css(this.tooltip, 'margin-top', -this.tooltip.offsetHeight / 2 + 'px');
 					} else {
-						this._css(this.tooltip, 'margin-left', -this.tooltip.offsetWidth / 2 + 'px');
+						this._css(this.tooltip, 'margin-'+this.stylePos, -this.tooltip.offsetWidth / 2 + 'px');


could you rewrite this using an ES6 template string?

`margin-${this.stylePos}`

rovolution · 2017-01-02T22:33:05Z

src/js/bootstrap-slider.js

@@ -1804,7 +1851,7 @@ const windowIsDefined = (typeof window === "object");
 			_setTooltipPosition: function(){
 				var tooltips = [this.tooltip, this.tooltip_min, this.tooltip_max];
 				if (this.options.orientation === 'vertical'){
-					var tooltipPos = this.options.tooltip_position || 'right';
+					var tooltipPos = this.options.tooltip_position || (this.options.rtl ? 'left' : 'right');


could you refactor this to an if..else since the logic to determine the variable's value is a little more complex?

var tooltipPos; if (this.options.tooltip_position) { tooltipPos = this.options.tooltip_position; } else if (this.options.rtl) { tooltipPos = 'left'; } else { tooltipPos = 'right'; }

Shnoulle · 2017-01-03T07:38:53Z

@rovolution : thanks for reviewing :) 😃 . Maybe about RtlOptionsSpec i need your help after .

I hope to work on it before end of the week :).

And thanks a lot for this repo

- [x] rewrite using ES6 template string - [x] refactor imbricated ternary test

- [x] RtlOptionsSpec unit test

Shnoulle · 2017-01-04T00:09:36Z

rewrite using ES6 template string
refactor imbricated ternary test
RtlOptionsSpec unit test

Attention : using rtl : 'auto' by default update API (different behaviour in a rtl page) then can make it to false by default then don't break API

rovolution · 2017-01-04T02:00:33Z

test/specs/RtlOptionsSpec.js

+      expect(dirIsRtl).toBeTruthy();
+    });
+
+    it("slider use inversed left and right inline style", function() {


great tests!!!!

rovolution · 2017-01-04T02:28:26Z

Just pulled down and ran locally and everything works great! merging now. Thanks again for this awesome PR! Will be available in v9.6.0 once I merged/publish to NPM

rovolution · 2017-01-04T02:35:42Z

merged and published to NPM at v9.6.0

Shnoulle · 2017-01-04T07:35:34Z

Thanks to you :)

Shnoulle added 4 commits December 29, 2016 19:59

[x] remove uneeded null in window.getComputedStyle

981b87c

Update README + add tpl

d389491

[x] documentation updates to README file [x] examples within [/tpl/index.tpl](https://github.com/seiyria/bootstrap-slider/blob/master/tpl/index.tpl)

Fix vertical tooltip_position

e4f8538

Fix tick label in vertical orientation

f77d0dd

Shnoulle commented Dec 30, 2016

View reviewed changes

rovolution requested changes Jan 2, 2017

View reviewed changes

rovolution added the incorporate-feedback label Jan 3, 2017

Shnoulle added 2 commits January 4, 2017 00:11

ES6 template + less ternary test

610c939

- [x] rewrite using ES6 template string - [x] refactor imbricated ternary test

Adding some jasmine specs

09adf60

- [x] RtlOptionsSpec unit test

rovolution reviewed Jan 4, 2017

View reviewed changes

rovolution approved these changes Jan 4, 2017

View reviewed changes

rovolution removed the incorporate-feedback label Jan 4, 2017

rovolution merged commit 38fd966 into seiyria:master Jan 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature : rtl mode (with options) #679

Feature : rtl mode (with options) #679

Shnoulle commented Dec 30, 2016 •

edited

Loading

seiyria commented Dec 30, 2016

Shnoulle commented Dec 30, 2016

seiyria commented Dec 30, 2016

Shnoulle Dec 30, 2016

Shnoulle Dec 30, 2016

Shnoulle commented Dec 30, 2016 •

edited

Loading

rovolution left a comment •

edited

Loading

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

rovolution Jan 2, 2017

Shnoulle commented Jan 3, 2017

Shnoulle commented Jan 4, 2017

rovolution Jan 4, 2017

rovolution commented Jan 4, 2017

rovolution commented Jan 4, 2017

Shnoulle commented Jan 4, 2017

Feature : rtl mode (with options) #679

Feature : rtl mode (with options) #679

Conversation

Shnoulle commented Dec 30, 2016 • edited Loading

Pull Requests

Feedback

seiyria commented Dec 30, 2016

Shnoulle commented Dec 30, 2016

seiyria commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shnoulle commented Dec 30, 2016 • edited Loading

rovolution left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shnoulle commented Jan 3, 2017

Shnoulle commented Jan 4, 2017

Choose a reason for hiding this comment

rovolution commented Jan 4, 2017

rovolution commented Jan 4, 2017

Shnoulle commented Jan 4, 2017

Shnoulle commented Dec 30, 2016 •

edited

Loading

Shnoulle commented Dec 30, 2016 •

edited

Loading

rovolution left a comment •

edited

Loading