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

Remove animation if user prefers reduced motion #2586

Merged
merged 3 commits into from Jun 29, 2020
Merged

Remove animation if user prefers reduced motion #2586

merged 3 commits into from Jun 29, 2020

Conversation

icncsx
Copy link
Contributor

@icncsx icncsx commented Jun 8, 2020

Description

This PR addresses issue #2509.

Per the guidance of @mstange, we are making a revision that makes sure there is no animation if the prefers-reduced-motion is active; otherwise, the animation is going to be a simple downwards translation.

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #2586 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2586      +/-   ##
==========================================
+ Coverage   86.55%   86.57%   +0.01%     
==========================================
  Files         217      217              
  Lines       17168    17079      -89     
  Branches     4433     4405      -28     
==========================================
- Hits        14860    14786      -74     
+ Misses       2112     2099      -13     
+ Partials      196      194       -2     
Impacted Files Coverage Δ
src/components/timeline/ActiveTabResourceTrack.js 81.63% <0.00%> (-1.95%) ⬇️
src/actions/app.js 78.94% <0.00%> (-1.36%) ⬇️
src/profile-logic/sanitize.js 98.93% <0.00%> (-0.07%) ⬇️
src/profile-logic/marker-data.js 93.01% <0.00%> (-0.03%) ⬇️
src/profile-logic/profile-data.js 89.95% <0.00%> (-0.03%) ⬇️
src/reducers/app.js 98.38% <0.00%> (-0.02%) ⬇️
src/reducers/url-state.js 96.81% <0.00%> (-0.02%) ⬇️
src/utils/string.js 100.00% <0.00%> (ø)
src/components/app/MenuButtons/Publish.js 75.72% <0.00%> (ø)
src/components/sidebar/CallTreeSidebar.js 93.75% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20826c6...21429da. Read the comment docs.

@icncsx icncsx changed the title Remove animation if user Remove animation if user prefers reduced motion Jun 8, 2020
@mstange mstange requested a review from gregtatum June 8, 2020 17:04
@mstange
Copy link
Contributor

mstange commented Jun 8, 2020

Looks great to me! I've also tested the new animation and I think it looks good. It has a bit of an initial opening delay, but that's due to the drop shadow SVG filter and is caused by a performance bug in Firefox, and not something that we need to fix in the profiler.

Greg, can you review? I suggested most of this implementation so I think this could use another pair of eyes.

Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

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

Thanks, yeah this works for me. With the delay the animation is almost hard to see. I wonder if it's worth making it faster by removing the SVG filter trick. I did a box shadow and it was a lot snappier. I also manually tested the reduced motion, and it worked on my end.

@mstange
Copy link
Contributor

mstange commented Jun 8, 2020

I'm fine with using a box-shadow. It'll look a bit odd around the arrow but it should be good enough.

@icncsx
Copy link
Contributor Author

icncsx commented Jun 9, 2020

Thank you for advising, Markus and Greg :) Happy to contribute more in the future.

@gregtatum gregtatum merged commit 5a83ee2 into firefox-devtools:master Jun 29, 2020
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.

None yet

3 participants