Skip to content

Add Math.round for positions and dimensions #454

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 7 commits into from
Nov 9, 2016
Merged

Add Math.round for positions and dimensions #454

merged 7 commits into from
Nov 9, 2016

Conversation

dmytro-krekota
Copy link
Contributor

By working with this library angularjs-slider library I ran into a problem, that some positions and dimensions sometimes set as 111.333px (not integer value) and browsers render my .svg images differently (sometimes blurry). See the screenshot:
https://drive.google.com/file/d/0Bxtv10NlSOEbM1d5cHZ4VnJwTWM/view?usp=sharing
So, I suggest to add Math.round calls in some places. The decision forces browsers work equally and solves my problems.

@ValentinH
Copy link
Member

Thanks for contributing this, I would be happy to merge it. Before, you need to fix the unit tests and generate the dist files. You can read the CONTRIBUTING.md file to help you if you want.

dmitry_krekota added 3 commits November 8, 2016 13:40
…e correct position for labels for single slider with boundPointerLabels=false"
…e correct position for labels for single slider with boundPointerLabels=false"
@codecov-io
Copy link

codecov-io commented Nov 8, 2016

Current coverage is 100% (diff: 100%)

Merging #454 into master will not change coverage

@@           master   #454   diff @@
====================================
  Files           1      1          
  Lines         921    921          
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
  Hits          921    921          
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update e2f720a...b99915b

@dmytro-krekota
Copy link
Contributor Author

Done. I did all that you asked of me. Please check difference.

@@ -981,7 +981,7 @@
var tick = {
selected: self.isTickSelected(value),
style: {
transform: translate + '(' + position + 'px)'
transform: translate + '(' + Math.round(position) + 'px)'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this one is useless because you already patched the valueToPosition method.

var expectedDimension = Math.floor(helper.slider.valueToPosition(6)),
actualDimension = Math.floor(helper.slider.selBar[0].getBoundingClientRect().width);
var expectedDimension = helper.slider.valueToPosition(6),
actualDimension = helper.slider.selBar[0].getBoundingClientRect().width + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to add 1? It seems weird...

@dmytro-krekota
Copy link
Contributor Author

I removed Math.round from valueToPosition method, because of weird code in tests. So now, after obtaining value, you decide by yourself use Math.round or Math.floor. Please check again.

@ValentinH
Copy link
Member

Thanks for this. Math.floor was a trick to handle decimal value but it should not be useful anymore thanks to your update in code.

Do the tests pass if you put Math.round in valueToPosition and remove it from the updateTicks one? Indeed, it would be cleaner IMO

@dmytro-krekota
Copy link
Contributor Author

Do the tests pass if you put Math.round in valueToPosition and remove it from the updateTicks one?

No, some tests fail. Therefore, I propose to leave as is.

…slider

# Conflicts:
#	dist/rzslider.min.css
#	dist/rzslider.min.js
@ValentinH
Copy link
Member

Thanks for rebasing.

@ValentinH ValentinH merged commit 62d857d into angular-slider:master Nov 9, 2016
@ValentinH
Copy link
Member

Released via 5.8.7

@dmytro-krekota
Copy link
Contributor Author

dmytro-krekota commented Nov 9, 2016

Thanks for your activity

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