-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fixes #168, scrollbutton glitch, tests added #169
base: master
Are you sure you want to change the base?
Conversation
This fixes PolymerElements#168. When using Browser-Zoom scrollLeft can be a decimal number. Using Math.ceil on scrollLeft will ensure that the condition will be met in this case and the rightHidden will be set properly.
scrollable test-page
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
CLAs look good, thanks! |
I fixed the tests, so they work in shady & shadow dom-mode. I have tested Chrome, IE11 and Firefox locally. |
|
||
}); | ||
|
||
suite('zoomed, scrollable paper-tabs', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be wrapped in something like
if (Object.keys(document.createElement('div').style).indexOf('zoom') >= 0) {
so that it doesn't attempt these if the browser doesn't support zoom
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder :)
LGTM, except for the comment I left about testing if |
Thank you. You just gave me the motivation, to try some more stuff out ;-) |
I meant that the test suite itself should be wrapped. You don't actually need to remove them but, if the browser doesn't support the |
Yes, the test is now the same, as the other ones. I'm trying to exclude piece, by piece to track down the problem, because i cannot reproduce it locally. Currently, there is no use of the zoom-property at all. Perhaps there is some structural problem. I'll try some more. |
Oh! This isn't a problem with your code: Firefox and our test setup aren't playing well together right now. Don't worry about making the Firefox CI tests work. Until we get the underlying issue resolved, it's fine as long as it passes locally. |
Yes, that would have been my final conclusion aswell. I'll put it back together then. :-) |
Should be back to where i started again. |
This fixes #168. When using Browser-Zoom scrollLeft can be a fractional number. Using Math.ceil on scrollLeft ensures that the condition will be met properly.