Skip to content

Hide floor/ceil label when overlapped on combo label #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 29, 2016

Conversation

Liooo
Copy link
Contributor

@Liooo Liooo commented Jun 29, 2016

@codecov-io
Copy link

codecov-io commented Jun 29, 2016

Current coverage is 100%

Merging #357 into master will not change coverage

@@           master   #357   diff @@
====================================
  Files           1      1          
  Lines         806    811     +5   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits          806    811     +5   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last updated by d543e52...47792d9

@ValentinH
Copy link
Member

Thanks for taking care of that!
I think you can remove the cmbLabelShown property because it's never read.
Also, it will be good to add a test case covering this bug. If you want, I can point you the place the do it ;)

@ValentinH
Copy link
Member

For the unit test, you can add a case in this file: https://github.com/angular-slider/angularjs-slider/blob/master/tests/specs/labels-test.js

@Liooo Liooo force-pushed the fix-combo-label-piling-up branch from 24e3dbb to 47792d9 Compare June 29, 2016 07:14
@Liooo
Copy link
Contributor Author

Liooo commented Jun 29, 2016

cmbLabelShown is actually used here ;)

in order to test this case accurately, we need to take care of viewport size actually I think. The ideal test scenario is to ensure the floor/ceil label is hidden when

  • max/min label element is hidden and combo label is shown
  • max/min label element (which is invisible) IS NOT overlapping the floor/ceil label
  • cmb label element IS overlapping the floor/ceil label

, which is not that easy to setup. Possibly we could set the phantomjs's viewport size before test setup and get the translate function to return the "correct" length string.

The added test sets translation that returns loong string and expect cmb label to be hidden. But the same test also passes in the previous library version, because min/max label elements overlap floor/ceil labels either way.

(same thing applies to any label tests where the handle is located at middle, precisely speaking)

@ValentinH
Copy link
Member

My bad, I was on mobile this morning and miss it and also the unit test you added :)

@ValentinH
Copy link
Member

LGTM 👍

@ValentinH ValentinH merged commit 6e8b022 into angular-slider:master Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants