hover over ticks to view their value #638

snurby7 · 2016-10-18T16:58:23Z

Pull Requests

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

unit tests (we use Jasmine 1.3)
JSFiddle (Plunker link)
Link to original Github issue (Feature Request Feature Request: Show tooltip on tick mark hover #632 )
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 · 2016-10-18T16:59:25Z

src/js/bootstrap-slider.js

@@ -398,6 +398,21 @@ const windowIsDefined = (typeof window === "object");
 				}
 			}

+			function createTickMouseOverListener(reference, tick, index){
+                tick.addEventListener("mouseenter", function(){


please fix this indentation

seiyria · 2016-10-18T17:00:31Z

high quality PR! very nice job. my only concern I saw at a glance was the indentation. @rovolution will have more comments I'm sure.

snurby7 · 2016-10-18T17:03:29Z

thanks @seiyria!

rovolution

@snurby7 looks great! I left some feedback in my PR so once that is addressed just tag me again.

rovolution · 2016-10-18T18:25:59Z

src/js/bootstrap-slider.js

@@ -398,6 +398,21 @@ const windowIsDefined = (typeof window === "object");
 				}
 			}

+			function createTickMouseOverListener(reference, tick, index){
+				tick.addEventListener("mouseenter", function(){
+					var tempState = $.extend(true, reference._state, {});


can you write this without JQuery? One of this libraries requirements is that it should work in environments without JQuery

rovolution · 2016-10-18T18:26:06Z

src/js/bootstrap-slider.js

+			function createTickMouseOverListener(reference, tick, index){
+				tick.addEventListener("mouseenter", function(){
+					var tempState = $.extend(true, reference._state, {});
+					var idString = index >= 0 ? index : $(this).attr('aria-valuenow');


can you write this without JQuery? One of this libraries requirements is that it should work in environments without JQuery

rovolution · 2016-10-18T18:26:17Z

src/js/bootstrap-slider.js


 			********************************/
 			_removeSliderEventHandlers: function() {
 				// Remove keydown event listeners
 				this.handle1.removeEventListener("keydown", this.handle1Keydown, false);
 				this.handle2.removeEventListener("keydown", this.handle2Keydown, false);

+				if(this.options.ticks_tooltip){
+					$('.slider-tick-container div').each(function(){


can you write this without JQuery? One of this libraries requirements is that it should work in environments without JQuery

Could you also add a comment above this section indicating that you are removing the hover event listeners from the ticks?

snurby7 · 2016-10-18T20:14:04Z

@rovolution updated per your requests to remove the jQuery, sorry for all the commits.

rovolution

@snurby7 left some further feedback

rovolution · 2016-10-18T21:55:08Z

src/js/bootstrap-slider.js


 			********************************/
 			_removeSliderEventHandlers: function() {
 				// Remove keydown event listeners
 				this.handle1.removeEventListener("keydown", this.handle1Keydown, false);
 				this.handle2.removeEventListener("keydown", this.handle2Keydown, false);

+				//rmove the listeners from the ticks if they had their own listeners
+				if(this.options.ticks_tooltip){
+					var ticks = document.getElementsByClassName('slider-tick-container')[0].children;


this will work fine if there is only 1 slider in the DOM, but what if multiple slider instances are in the DOM? could you refactor this so that it only selects the ticks within this slider instance's .slider-tick-container?

On line 481, we bind the element reference for the tick container to the this.ticksContainer instance variable.

Therefore, should be able to access the ticks for a specific slider instance like so:

var ticks = this.ticksContainer.getElementsByClassName(".slider-tick");

rovolution · 2016-10-18T22:01:09Z

src/js/bootstrap-slider.js

+				if(this.options.ticks_tooltip){
+					var ticks = document.getElementsByClassName('slider-tick-container')[0].children;
+					for(var i = 0; i < ticks.length; i++ ){
+						ticks[i].removeEventListener('mouseenter', this.createTickMouseOverListener, false);


So this is not actually removing the mouseenter and mouseleave event listeners from the ticks.

Within the this._createTickMouseOverListener function, you are binding event listeners for the events. However, when removeEventListener is called, you need to pass in the specific function reference for the actual listener itself.

For example:

// Create a function reference var myEventListener = function() { console.log("yolo"); }; // Create an event listener by binding a DOM event to the specific function reference myElementReference.addEventListener("mouseenter", myEventListener); // This line properly removes the listener because myEventListener refers to the same function reference in memory that was originally bound in the line above myElementReference. removeEventListener("mouseenter", myEventListener, false);

this is what is currently happening:

// This method is acting similar to your createTickMouseOverListener function bindListener(element) { element.addEventListener("mouseenter", function() { console.log("yolo"); }); } // call your function bindListener(myElementReference); /* This will not actually remove the listener bound in the bindListener() function because the function references for `bindListener()` and `function() { console.log("yolo"); }` are different You need to find a way to pass the memory reference for the `function() { console.log("yolo"); }` function to the removeEventListener() call */ myElementReference.removeEventListener("mouseenter", bindListener, false);

@snurby7 Check out this article for further info: https://msdn.microsoft.com/en-us/library/dn741342(v=vs.94).aspx

snurby7 · 2016-10-19T15:41:28Z

@rovolution, fixed up to your comments, that was a fun experience!

rovolution

@snurby7 Added some more feedback.

In the _destroy() method, you will need to nullify the this.ticksCallbackMap and this.handleCallbackMap so that the objects they reference are marked for garbage collection.

this.ticksCallbackMap = null;
this.handleCallbackMap = null;

Could you also fix the spacing issues as well? Thanks!

rovolution · 2016-10-20T03:20:19Z

src/js/bootstrap-slider.js

+							var tickListenerReference = this._addTickListener();
+                            var enterCallback = tickListenerReference.addMouseEnter(this, tick, i);
+                            var leaveCallback = tickListenerReference.addMouseLeave(this, tick);
+                            if(i === 0){


So you need to actually map each tick index to an object that contains the function references for its callbacks:

Currently, you are just overwriting the this.ticksCallbackMap variable on each iteration of the loop

Try something like the following:

// Generate event listeners for tick var tickListenerReference = this._addTickListener(); var enterCallback = tickListenerReference.addMouseEnter(this, tick, i); var leaveCallback = tickListenerReference.addMouseLeave(this, tick); // Map tick index to event listeners this.ticksCallbackMap[i] = { mouseEnter: enterCallback, mouseLeave: leaveCallback };

rovolution · 2016-10-20T03:22:58Z

src/js/bootstrap-slider.js

 					for(var i = 0; i < ticks.length; i++ ){
-						ticks[i].removeEventListener('mouseenter', this.createTickMouseOverListener, false);
-						ticks[i].removeEventListener('mouseleave', this.createTickMouseOverListener, false);
+						ticks[i].removeEventListener('mouseenter', this.ticksCallbackMap.mouseEnter, false);


If you map the tick index to the event listener references as I mentioned above, you should be able to do the following here in order to cleanup the references:

var tickEventListeners = this.ticksCallbackMap[i]; var tickElement = ticks[i]; tickElement.removeEventListener('mouseenter', tickEventListeners.mouseEnter, false); tickElement.removeEventListener('mouseleave', tickEventListeners.mouseLeave, false);

rovolution · 2016-10-20T03:29:14Z

src/js/bootstrap-slider.js

+                    var mouseLeave = callbackHandle.addMouseLeave(this, this.handle1);
+                    callbackHandle.addMouseEnter(this, this.handle2);
+                    callbackHandle.addMouseLeave(this, this.handle2);
+                    this.handleCallbackMap = {


likewise, need to map each handle to its event listeners.

I would recommend something like the following:

var callbackHandle = this._addTickListener(); // generate event listeners for handle1 and store in map var mouseEnter = callbackHandle.addMouseEnter(this, this.handle1); var mouseLeave = callbackHandle.addMouseLeave(this, this.handle1); this.handleCallbackMap["handle1"] = { mouseEnter: mouseEnter, mouseLeave: mouseLeave }; // generate event listeners for handle2 and store in map mouseEnter = callbackHandle.addMouseEnter(this, this.handle2); mouseLeave = callbackHandle.addMouseLeave(this, this.handle2); this.handleCallbackMap["handle2"] = { mouseEnter: mouseEnter, mouseLeave: mouseLeave };

rovolution · 2016-10-20T03:31:07Z

src/js/bootstrap-slider.js

 					}
-                }
+					this.handle1.removeEventListener('mouseenter', this.handleCallbackMap.mouseEnter, false);


need to remove listeners from handles.

Something like the following:

this.handle1.removeEventListener('mouseenter', this.handleCallbackMap["handle1"].mouseEnter, false); this.handle2.removeEventListener('mouseenter', this.handleCallbackMap["handle2"].mouseEnter, false); this.handle1.removeEventListener('mouseleave', this.handleCallbackMap["handle1"].mouseLeave, false); this.handle2.removeEventListener('mouseleave', this.handleCallbackMap["handle2"].mouseLeave, false);

…s up to be collected

snurby7 · 2016-10-20T13:26:01Z

@rovolution cleaned it up again to your comments, also went through and removed some of the old white spaces that were still in the file, might as well fix it while I was in there.

rovolution · 2016-10-20T14:02:28Z

@snurby7 check out #638 (comment)

rovolution · 2016-10-21T01:34:43Z

@snurby7 alright i just pulled your branch down locally and QA'ed it. Everything checks out.

Thanks again for making this PR and taking the time to incorporate my feedback!

rovolution · 2016-10-21T01:45:16Z

Merged and published to v9.3.0

Thanks again @snurby7!

hover over ticks to view their value

2fdb5a7

seiyria reviewed Oct 18, 2016

View reviewed changes

fix indentation in slider

fe8cab6

snurby7 mentioned this pull request Oct 18, 2016

add ticks_tooltip to angular wrapper seiyria/angular-bootstrap-slider#141

Merged

rovolution requested changes Oct 18, 2016

View reviewed changes

snurby7 added 4 commits October 18, 2016 16:06

removed jquery references per request

10961d6

forgot the comment when removing

e7c9aba

fixed some formatting

2998814

missed a reference.

8ce25f1

rovolution requested changes Oct 18, 2016

View reviewed changes

fix memory leaks caused by improper removal + PR comments

a005cc4

rovolution requested changes Oct 20, 2016

View reviewed changes

register each handle to it's own map ref, fixed formatting, set thing…

a6ff4d5

…s up to be collected

map all tick references as well

bb7c69b

rovolution approved these changes Oct 21, 2016

View reviewed changes

rovolution merged commit 8d8c702 into seiyria:master Oct 21, 2016

algodave mentioned this pull request Feb 23, 2017

Slide handle tooltip position broken when using ticks_tooltip #716

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hover over ticks to view their value #638

hover over ticks to view their value #638

snurby7 commented Oct 18, 2016

seiyria Oct 18, 2016

seiyria commented Oct 18, 2016

snurby7 commented Oct 18, 2016

rovolution left a comment

rovolution Oct 18, 2016

rovolution Oct 18, 2016

rovolution Oct 18, 2016

rovolution Oct 18, 2016

snurby7 commented Oct 18, 2016

rovolution left a comment

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading

snurby7 commented Oct 19, 2016

rovolution left a comment

rovolution Oct 20, 2016

snurby7 Oct 20, 2016

rovolution Oct 20, 2016

rovolution Oct 20, 2016

rovolution Oct 20, 2016

snurby7 commented Oct 20, 2016

rovolution commented Oct 20, 2016

rovolution commented Oct 21, 2016

rovolution commented Oct 21, 2016

hover over ticks to view their value #638

hover over ticks to view their value #638

Conversation

snurby7 commented Oct 18, 2016

Pull Requests

Choose a reason for hiding this comment

seiyria commented Oct 18, 2016

snurby7 commented Oct 18, 2016

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

snurby7 commented Oct 18, 2016

rovolution left a comment

Choose a reason for hiding this comment

rovolution Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

rovolution Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

rovolution Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

rovolution Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

snurby7 commented Oct 19, 2016

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

snurby7 commented Oct 20, 2016

rovolution commented Oct 20, 2016

rovolution commented Oct 21, 2016

rovolution commented Oct 21, 2016

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading

rovolution Oct 18, 2016 •

edited

Loading