Skip to content
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

Use transform: translate() instead of margin-* for the tooltips. Fix #483 #798

Merged
merged 4 commits into from
Nov 9, 2017

Conversation

wismill
Copy link

@wismill wismill commented Nov 2, 2017

Fix #483 using CSS property transform instead of margin-* for the tooltips.

According to caniuse.com, this property has a very good support in the browsers.

@seiyria
Copy link
Owner

seiyria commented Nov 2, 2017

this would need to pass our full CI suite before we could merge it. You'll also need to follow the contribution guidelines: https://github.com/seiyria/bootstrap-slider/blob/master/.github/CONTRIBUTING.md

You need to add a unit test to replace the one that was taken away, and I would like to see a jsfiddle of this.

@wismill
Copy link
Author

wismill commented Nov 2, 2017

Well, it passes the full CI as I did not modify any of the lines reported by codeclimate.
I do follow your contribution guidelines. Please indicate what is not respected.
The unit test does not apply anymore, as the margin of the margins of the tooltips are never modified.

@seiyria
Copy link
Owner

seiyria commented Nov 2, 2017

  • It will need to pass code climate, regardless of whether you touched it or not
  • You should add a jsfiddle so we can see it in action
  • You should add a new unit test in place of the old one.

@wismill
Copy link
Author

wismill commented Nov 2, 2017

The issue detected by codeclimate is a duplication between javascript files in src and dist. How can I solve this?

@seiyria
Copy link
Owner

seiyria commented Nov 2, 2017 via email

@wismill
Copy link
Author

wismill commented Nov 2, 2017

JSFiddle:

@wismill
Copy link
Author

wismill commented Nov 2, 2017

Re-added the test, modified.

@rovolution
Copy link
Collaborator

My one concern is that since we state in our docs that we support IE9 and above, this will break that guarantee.

That being said, I would be ok with merging/releasing this, but would want to do so under a major version bump.

@wismill
Copy link
Author

wismill commented Nov 8, 2017

It is supported by IE 9 using prefix -ms-. But this will add more code. Or all of this could be managed in CSS by using a class rather than a fixed CSS property using Javascript. I'll check that.

@wismill
Copy link
Author

wismill commented Nov 8, 2017

I have simplified the code and added support for IE 9. In fact, your code already uses CSS transform for .slider-handle.triangle and without compatibility.

@rovolution
Copy link
Collaborator

rovolution commented Nov 9, 2017

The issue detected by codeclimate is a duplication between javascript files in src and dist. How can I solve this?

Yea I agree this is a red-herring error. I added the dist directory contents to the exclude_paths section of the codeclimate config file, so this should prevent any errors coming from that directory going forward.

@rovolution rovolution merged commit 32cf0f3 into seiyria:master Nov 9, 2017
@rovolution
Copy link
Collaborator

merged and released to v10.0.0 (published as a breaking change just incase there any inadvertent regressions). Thanks again for your contribution!

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