-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Using transform instead of box-shadow for user location dot pulse animation #5498
Using transform instead of box-shadow for user location dot pulse animation #5498
Conversation
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.
Thank you for contributing to mapbox gl js @jasonbarry! Changes look good to me, and 👍 on using hardware accelerated css animations where possible. I will download the new Safari tonight to try and reproduce – I'm not seeing the weird square flash in Safari 10.
@andrewharvey could you take a look at this PR since you're the original author?
@jasonbarry 👍 I'm onboard with these changes, however when I tested this change out on Chrome 61 with https://jsbin.com/poriqe/edit?html,output it looks off... I can try to help out with checking these changes in IE11, Android browser, Firefox. |
@andrewharvey Thanks for the feedback! Fixed that issue and made some minor adjustments, see here: https://jsbin.com/qudibixica/edit?html,output Tested in latest Safari, Firefox, and Opera on macOS. Would definitely appreciate more cross-browser testing. |
@jasonbarry Looks better now in Chrome. I tested on IE11, it looks like it's off centre a bit (inside dot vs the animated halo). It's not a big issue, but does look like a step back compared to how it currently looks in IE11. |
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.
I just did a bunch of testing on different devices and browsers with this and it seems fine, not sure why I was seeing it offset in IE11 before, but I'm happy with this PR. Thanks @jasonbarry
Thanks @andrewharvey – I just tried to test in IE using browserstack but I ran into the same issue I was having in October with the Geolocation plugin not working. |
@mollymerp I think browserstack is a bit hit and miss, but I did managed to see it looking good in both browserstack Windows 7 IE11 and on a real Windows 7 IE11, along with ios safari 11 and a bunch of latest android and desktop browsers. |
In Safari 11.0, I noticed that the Locate the user example looked like this:
I modified the CSS to use transforms instead of box-shadow for the pulse animation, which not only fixes the square-flash issue, but is also more performant because it's hardware-accelerated.
17px15px — odd diameter widths look better when centeringanimation
properties to eliminate the need foranimation-iteration-count
properties