lock_to_ticks #744

heidarsaleh · 2017-04-28T23:40:37Z

Pull Requests

Please accompany all pull requests with the following (where appropriate):

unit tests (we use Jasmine 2.x.x)
JSFiddle (or an equivalent such as CodePen, Plunker, etc) or screenshot/GIF with new feature or bug-fix
Link to original Github issue (if this is a bug-fix)
documentation updates to README file
examples within /tpl/index.tpl (for new options being added)
Passes CI-server checks (runs automated tests, JS source-code linting, etc..). To run these on your local machine, type grunt test in your Terminal within the bootstrap-slider repository directory

seiyria · 2017-04-29T00:49:38Z

please:

make sure your code passes our CI
add unit tests
link to the issue this is referencing
update the documentation
add an example

heidarsaleh · 2017-05-05T21:21:07Z

Issue: #737

heidarsaleh · 2017-05-05T21:22:56Z

Adding lock_to_ticks: true will lock the selection to the values defined inside "ticks"
Example:

$("#id_size_select").slider({
                tooltip: 'always',
                tooltip_position: 'top',
                step: 10,
                ticks: [1, 10, 50, 200, 500, 1000, 5000, 10000],
                ticks_positions: [1, 14.28, 28.56, 42.84, 57.12, 71.4, 85.68, 100],
                ticks_snap_bounds: 5000,
                lock_to_ticks: true        //now user will only be able to select 1, 10, 50, 200, 500, 1000, 5000 or 10000
            });

seiyria · 2017-05-05T21:40:02Z

For the example, please add it to github pages branch.

heidarsaleh · 2017-05-05T22:08:42Z

Example has been added to tpl/index.tpl. Should I add it anywhere else ?

heidarsaleh · 2017-05-05T22:13:35Z

Ah, gh-pages branch. Will do it now

heidarsaleh · 2017-05-05T22:34:35Z

done. Please let me know If I'm missing something

rovolution

@heidarmostafa looks solid! could you post a JSFiddle or codepen with the changes just so we can get a quick glance as to what this functionality provides?

I also left some comments in the PR

rovolution · 2017-05-08T15:41:18Z

dist/css/bootstrap-slider.css

@@ -1,308 +0,0 @@
-/*! =======================================================
-                      VERSION  9.8.0              


can you remove any changes to the dist directory from this PR? this are compiled files that get generated when we publish a new version

rovolution · 2017-05-08T15:42:16Z

src/js/bootstrap-slider.js

+							this._state.value[1] = this.options.ticks[this._getClosestTickIndex(this._state.value[1])];
+						}
+					} catch(err) {
+						console.error(err);


why would this code block throw an error?

rovolution · 2017-05-08T15:42:40Z

src/js/bootstrap-slider.js

@@ -1078,12 +1089,10 @@ const windowIsDefined = (typeof window === "object");
 				this.ticksCallbackMap = null;

 				if (this.showTooltip) {
-					this.handle1.removeEventListener("focus", this.showTooltip, false);
-					this.handle2.removeEventListener("focus", this.showTooltip, false);
+					this._removeTooltipListener("focus");


nice refactoring!! 👍

rovolution · 2017-05-08T15:43:09Z

src/js/bootstrap-slider.js

@@ -1682,28 +1691,29 @@ const windowIsDefined = (typeof window === "object");

 				return false;
 			},
+			_setValues: function(index, val) {
+				var comp = index === 0 ? 0 : 100;


could you use an ES6 const declaration instead of var? also, wrap the conditional in parenthesis to make it stand out more easily

rovolution · 2017-05-08T15:44:39Z

src/js/bootstrap-slider.js

@@ -1907,6 +1917,18 @@ const windowIsDefined = (typeof window === "object");
 						tooltip.style.top = -this.tooltip.outerHeight - 14 + 'px';
 					}.bind(this));
 				}
+			},
+			_getClosestTickIndex: function(val) {
+				var difference = Math.abs(val - this.options.ticks[0]);


could you declare these vars as let instead of var?

rovolution · 2017-05-08T15:44:52Z

src/js/bootstrap-slider.js

+			_getClosestTickIndex: function(val) {
+				var difference = Math.abs(val - this.options.ticks[0]);
+				var index = 0;
+				for (var i = 0; i < this.options.ticks.length; ++i) {


declare the i as let instead of var

rovolution · 2017-05-08T15:47:05Z

test/specs/LockToTicksSpec.js

+		testSlider.slider("setValue", [4, 102]);
+
+		//getting the actual values. They should be the closest values from ticks (which are 1 and 50 on this case)
+		var values = testSlider.slider("getValue");


nice test btw!

could you also add a test case for a vertical slider?

heidarsaleh

Done. Please have a final review and let me know if something is missing. Thanks.

heidarsaleh · 2017-05-08T20:07:15Z

I rolled back the dist folder. Now codeclimate detect duplicate between the src and the dist. We need to ignore this error I think.

rovolution · 2017-05-08T20:17:35Z

Now codeclimate detect duplicate between the src and the dist

Don't worry too much about the codeclimate check...I'm more concerned with the Travis CI check (which looks like its passing)

heidarsaleh · 2017-05-08T20:20:46Z

jsfiddle: https://jsfiddle.net/vk2q7rxb/

heidarsaleh · 2017-05-08T20:54:18Z

Yep. Can you merge it now? I'd also be happy to work on other issues or planned features :)

rovolution · 2017-05-08T21:02:49Z

src/js/bootstrap-slider.js

@@ -1682,28 +1687,34 @@ const windowIsDefined = (typeof window === "object");

 				return false;
 			},
+			_setValues: function(index, val) {
+				let comp;
+				if(0 === index) {


keep this as a ternary, but just declare it as a const

const comp = (0 === index) ? 0 : 100;

rovolution

@heidarmostafa left some other minor feedback

rovolution · 2017-05-08T21:13:36Z

src/sass/_rules.scss

@@ -5,6 +5,7 @@
  &.slider-horizontal {
    width: $slider-horizontal-width;
    height: $slider-line-height;
+    margin: 0 0 010px;


could you fix this?

rovolution · 2017-05-08T21:13:51Z

test/specs/LockToTicksSpec.js

+		testSlider.slider("setValue", [4, 102]);
+
+		//getting the actual values. They should be the closest values from ticks (which are 1 and 50 on this case)
+		var values = testSlider.slider("getValue");


could you also add a test case for a vertical slider?

heidarsaleh

Done

rovolution

@heidarmostafa I just cloned down your branch and ran it locally and it looks great!

I found one final thing during QA...If I use the Tab key to get the focus on one of the handles, and then try to move the handle with my arrow keys, it does not move to the next tick.

Steps to Replicate:
1) Load up a page with the slider configured with the lock_to_ticks option.
2) Use the Tab key to get the focus onto the handle. A handle with focus has a blue outline, and should look like this in Chrome:

3) Press arrow keys to move the slider.

Expected behavior: handle snaps to nearest tick based on arrow key press

current behavior: handle remains in place

rovolution · 2017-05-15T15:58:01Z

@heidarsaleh did you get a chance to see my feedback? other than that, it looks great!

heidarsaleh · 2017-05-16T07:45:23Z

Sorry I wasn't available the past few days. I'll fix it this weekend.

rovolution · 2017-05-16T14:35:03Z

@heidarsaleh no rush! whenever you get a chance

rovolution · 2017-05-30T16:53:36Z

@heidarsaleh any chance to look at my feedback? I would love to merge this in!

rovolution · 2017-06-23T06:14:57Z

@heidarsaleh any chance to check this out?

heidarsaleh · 2017-06-25T14:43:04Z

I've checked it Today.This feature will require a lot of changes in different places. I'll try to fix it soon.

neslinesli93 · 2018-02-01T14:31:28Z

any update on this?

rovolution · 2018-02-02T15:53:37Z

@heidarsaleh i know its been a while, but did you even get a chance to look at this again?

Merged changes from: seiyria#873

Added `tickIndex` to slider state which is an array of which ticks the slider handle(s) are set (can be -1). The array is computed in the `_setTickIndex()` function. This can be useful for setting the value(s) of the slider based on `options.ticks[]`.

Test for cases when `ticks[]` is not defined and when `setAttribute()` may be called during runtime.

Also added comments to help explain the mouse navigation tests.

Also added comments to help explain keyboard navigation tests

This fix is related to seiyria#896 which calls `setValue()` in `_mouseup()`.

jespirit · 2018-12-21T16:29:53Z

src/js/bootstrap-slider.js

+				var val;
+				if (this.ticksAreValid && this.options.lock_to_ticks) {
+					var index;
+					try {


@rovolution I use a try-catch block but there is no "real" exception occuring. I just check index === -1 which shouldn't happen and fallback to setting index to 0. I can change this to a regular if-else check.

jespirit · 2019-01-04T00:09:34Z

I decided to replace the try/catch block with an simple if statement and also prints a console warning.

I've manually tested the API examples and everything seems to work

rovolution · 2019-01-04T04:31:41Z

@jespirit does the keyboard stuff I mentioned in this comment work as well?

jespirit · 2019-01-04T04:33:40Z

@jespirit does the keyboard stuff I mentioned in this comment work as well?

Sure does!

heidarsaleh mentioned this pull request Apr 29, 2017

Feature request: strict ticks. #737

Open

heidarsaleh force-pushed the master branch from 547dab7 to 8369a20 Compare May 5, 2017 22:20

rovolution requested changes May 8, 2017

View reviewed changes

rovolution reviewed May 8, 2017

View reviewed changes

heidarsaleh commented May 8, 2017

View reviewed changes

rovolution requested changes May 8, 2017

View reviewed changes

heidarsaleh commented May 8, 2017

View reviewed changes

rovolution approved these changes May 9, 2017

View reviewed changes

rovolution requested changes May 9, 2017

View reviewed changes

rovolution added the incorporate-feedback label Oct 23, 2017

jespirit and others added 21 commits December 21, 2018 11:12

optimization: _setValues

7ae5537

Merged changes from: seiyria#873

optimization: getValue

a1c8459

optimization: _removeTooltipListener

1ff8f96

updating documentation

67cfb6f

add unit test for lock_to_ticks option

a95775c

adding lock_to_ticks example to tpl/index.tpl

ef10f0d

removing extra character

fd45a77

Update README.md

4052f77

fixing code comments

cd53c29

fixing code comments seiyria#2

85d1290

Fix lock_to_ticks for single sliders

d58c368

Lock to ticks only when ticks[] is set

d7cf6ad

Fix arrow key functionality with RTL

e1ab434

Add lock_to_ticks unit tests

e8b36f2

Test for cases when `ticks[]` is not defined and when `setAttribute()` may be called during runtime.

Add lock_to_ticks unit tests using mouse navigation

7a11bef

Also added comments to help explain the mouse navigation tests.

Add lock_to_ticks unit tests using keyboard navigation

ea1f8b7

Also added comments to help explain keyboard navigation tests

Fix example 25 and add link to table

8fa2318

Fix ticks not locking on 'mouseup' event

3946329

Check that the positions of the handles are correct

b002a41

Remove check on _mouseup

290a2ea

This fix is related to seiyria#896 which calls `setValue()` in `_mouseup()`.

jespirit force-pushed the master branch from 3d90ba2 to 290a2ea Compare December 21, 2018 16:26

jespirit reviewed Dec 21, 2018

View reviewed changes

Replace try/catch block with if statement

21634e7

Fix examples 24 and lock-to-ticks initialization code

eee611e

rovolution approved these changes Jan 4, 2019

View reviewed changes

jespirit merged commit ccf5122 into seiyria:master Jan 4, 2019

		@@ -1,308 +0,0 @@
		/*! =======================================================
		VERSION 9.8.0

lock_to_ticks #744

lock_to_ticks #744

Conversation

heidarsaleh commented Apr 28, 2017

Pull Requests

seiyria commented Apr 29, 2017

heidarsaleh commented May 5, 2017

heidarsaleh commented May 5, 2017

seiyria commented May 5, 2017

heidarsaleh commented May 5, 2017

heidarsaleh commented May 5, 2017

heidarsaleh commented May 5, 2017 • edited Loading

rovolution left a 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

heidarsaleh left a comment • edited Loading

Choose a reason for hiding this comment

heidarsaleh commented May 8, 2017

rovolution commented May 8, 2017 • edited Loading

heidarsaleh commented May 8, 2017

heidarsaleh commented May 8, 2017

Choose a reason for hiding this comment

rovolution left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heidarsaleh left a comment

Choose a reason for hiding this comment

rovolution left a comment • edited Loading

Choose a reason for hiding this comment

rovolution commented May 15, 2017

heidarsaleh commented May 16, 2017

rovolution commented May 16, 2017

rovolution commented May 30, 2017

rovolution commented Jun 23, 2017

heidarsaleh commented Jun 25, 2017

neslinesli93 commented Feb 1, 2018

rovolution commented Feb 2, 2018

Choose a reason for hiding this comment

jespirit commented Jan 4, 2019

rovolution commented Jan 4, 2019

jespirit commented Jan 4, 2019 • edited Loading

heidarsaleh commented May 5, 2017 •

edited

Loading

heidarsaleh left a comment •

edited

Loading

rovolution commented May 8, 2017 •

edited

Loading

rovolution left a comment •

edited

Loading

jespirit commented Jan 4, 2019 •

edited

Loading