seiyria / bootstrap-slider

A slider control for Bootstrap 3 & 4.

http://seiyria.github.io/bootstrap-slider/

Other

3k stars 1.14k forks source link

Fixes Issue #884: Hovering over ticks displays the tick index #891

Closed jespirit closed 5 years ago

jespirit commented 5 years ago

Fixes #884

This displays the correct value for the tick and correctly positions the tooltip above the tick.

Will add unit tests.

Pull Requests

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

[x] unit tests (we use Jasmine 2.x.x)
[x] Link to original Github issue (if this is a bug-fix)
[ ] 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

jespirit commented 5 years ago

Added some unit test coverage that codeclimate wouldn't be too happy about it.

My unit tests before fix:

issue-884-unit-tests

Of course, then there's RTL and different positions of the tooltip... etc.

jespirit commented 5 years ago

I added more unit tests for displaying tooltips for a vertical slider.

My unit tests before fix:

issue-884-unit-tests-vertical

Codeclimate would be really unhappy. I essentially copied the same code for the horizontal slider except I changed the offsetLeft to be offsetTop.

rovolution commented 5 years ago

Codeclimate would be really unhappy. I essentially copied the same code for the horizontal slider except I changed the offsetLeft to be offsetTop.

Did you copy / paste for the unit tests? I don't really care about that TBH. if its too difficult to refactor into a sensible DRY format, then its not worth the hassle. Biggest thing is that we have test coverage on all potential cases + any bugs that arise so that we can prevent regressions.

jespirit commented 5 years ago

I copied and pasted more unit tests. You can check the commits.

I don't think I can refactor that stuff. I could try? It was hard to manage. One mistake and I had to change other parts of the code =(

I did notice that the ClientX and ClientY values are just ignored when dispatching mouseenter events when I was writing the tests for when hovering over the handles. I could probably (?) just get rid of the offsets entirely when using mouseenter for the other tests.

jespirit commented 5 years ago

~~As per your request/challenge~~ I refactored the code.

I tried to use as many variables as I can and moved as much initialization code inside beforeEach() functions.

In fact, I just copied all the tooltip unit tests and changed a few variables to test against the RTL slider.

Before my fix:

issue-884-all-unit-tests

Good luck, trying to maintain that code!

rovolution commented 5 years ago

LGTM 👍

jespirit commented 5 years ago

I refactored the unit tests so it's easier to manage.