Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd option to reduce motion #5393
Conversation
nolanlawson
requested a review
from
nightpool
Oct 14, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ilianaw
Oct 14, 2017
Hi! Thank you so much for this, and thanks for pointing to the "Responsive Design for Motion" blog post — it's helped me understand my own issues better :)
There is also a zoom animation when you select followers-only in the post privacy dropdown if your account is not locked to show the "anyone can follow you" disclaimer; I've not read the code closely enough to know if you've changed that too.
I also did not realize that there was a media query for a reduce motion option. I wonder if it's worth implementing that as well even though only Safari has support for it presently.
Do we want this to apply to all animations everywhere?
Probably not. As the resource you link describes, some animations trigger motion sickness; most of the examples replace motion with linear fades and those sort of feel fine to me.
Should we do a lightweight animation or no animation at all?
The "Don't Reduce Too Much" section in the WebKit resource makes sense to me; I think using a linear fade for the dropdowns is sufficient and may be better than no animation at all.
ilianaw
commented
Oct 14, 2017
|
Hi! Thank you so much for this, and thanks for pointing to the "Responsive Design for Motion" blog post — it's helped me understand my own issues better :) There is also a zoom animation when you select followers-only in the post privacy dropdown if your account is not locked to show the "anyone can follow you" disclaimer; I've not read the code closely enough to know if you've changed that too. I also did not realize that there was a media query for a reduce motion option. I wonder if it's worth implementing that as well even though only Safari has support for it presently.
Probably not. As the resource you link describes, some animations trigger motion sickness; most of the examples replace motion with linear fades and those sort of feel fine to me.
The "Don't Reduce Too Much" section in the WebKit resource makes sense to me; I think using a linear fade for the dropdowns is sufficient and may be better than no animation at all. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nightpool
Oct 14, 2017
Collaborator
|
Okay based on feedback I'm happy keeping this just to react motion stuff
for now, focusing on zooms and parallax. We should also probably disable
updating the timeline in real time but that's pretty difficult and out of
scope for the PR
Code wise, It would be much cleaner if we could apply this globally rather
then everywhere we use react motion—there's a lot of potential for
forgetting to include it or missing an edge case. I'm a fan of keeping it
in the global store but also interested in what other people who touch this
code more often think.
This would be great to get even a barebones version of into 2.0
…On Sat, Oct 14, 2017 at 2:22 PM iliana weller ***@***.***> wrote:
Hi! Thank you so much for this, and thanks for pointing to the "Responsive
Design for Motion" blog post — it's helped me understand my own issues
better :)
There is also a zoom animation when you select followers-only in the post
privacy dropdown if your account is not locked to show the "anyone can
follow you" disclaimer; I've not read the code closely enough to know if
you've changed that too.
I also did not realize that there was a media query for a reduce motion
option. I wonder if it's worth implementing that as well even though only
Safari has support for it presently.
Do we want this to apply to all animations everywhere?
Probably not. As the resource you link
<https://webkit.org/blog/7551/responsive-design-for-motion/> describes,
some animations trigger motion sickness; most of the examples replace
motion with linear fades and those sort of feel fine to me.
Should we do a lightweight animation or no animation at all?
The "Don't Reduce Too Much" section in the WebKit resource makes sense to
me; I think using a linear fade for the dropdowns is sufficient and may be
better than no animation at all.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#5393 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAORVwdujnSSHQYHBcntBySvfkOkngsCks5ssPvggaJpZM4P5dpr>
.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 14, 2017
Collaborator
@ilianaw OK I think I can rework this so it only does fades. Your explanation makes sense.
@nightpool Yeah we can maybe save CSS animations for a future PR, but for now it seems the ReactMotion ones are the worst offenders.
|
@ilianaw OK I think I can rework this so it only does fades. Your explanation makes sense. @nightpool Yeah we can maybe save CSS animations for a future PR, but for now it seems the ReactMotion ones are the worst offenders. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 14, 2017
Collaborator
OK, this new version solves a few problems at once:
- Instead of having to pass around
reduceMotionas a prop everywhere, just create a Higher Order Component aroundMotionand have it read from the Redux store - Because of this, we can just replace
Motioneverywhere it's used - Inside of the HOC, preserve
opacityandbackgroundOpacityanimations but remove all other animations (e.g.x,rotate,scale, etc.)
I don't think this feature is perfect yet: some animations look a little slow (e.g. the favorite star) and some look a little sudden (e.g. the search drawer, since it has no animation at all), but conceptually this is much simpler because we can just apply it to all uses of Motion. And perhaps in the future we can pass some custom options to this new HOC to tell it what kinds of animations to do when reduce_motion is true.
Here's an updated before video and after video. Note that the "Followers-only" modal is covered now.
|
OK, this new version solves a few problems at once:
I don't think this feature is perfect yet: some animations look a little slow (e.g. the favorite star) and some look a little sudden (e.g. the search drawer, since it has no animation at all), but conceptually this is much simpler because we can just apply it to all uses of Here's an updated before video and after video. Note that the "Followers-only" modal is covered now. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 14, 2017
Collaborator
Hmm it seems there may be a performance regression from adding the HOC. I'm going to investigate further.
|
Hmm it seems there may be a performance regression from adding the HOC. I'm going to investigate further. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 15, 2017
Collaborator
Yeah so apparently this HOC pattern is clever, but causes a problem where every single Motion component is updated whenever the state updates... Reading about reselect to see if that can help out.
|
Yeah so apparently this HOC pattern is clever, but causes a problem where every single |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 15, 2017
Collaborator
OK, the performance issue appears to be fixed by avoiding recreating new objects in mapStateToProps. This PR is ready for review.
|
OK, the performance issue appears to be fixed by avoiding recreating new objects in |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nolanlawson
Oct 15, 2017
Collaborator
This is live at https://malfunctioning.technology for testing.
|
This is live at https://malfunctioning.technology for testing. |
| @@ -25,4 +25,4 @@ | ||
| - video = status.media_attachments.first | ||
| %div{ data: { component: 'Video', props: Oj.dump(src: video.file.url(:original), preview: video.file.url(:small), sensitive: status.sensitive?, width: 610, height: 343) }} | ||
| - else | ||
| - %div{ data: { component: 'MediaGallery', props: Oj.dump(height: 343, sensitive: status.sensitive?, 'autoPlayGif': current_account&.user&.setting_auto_play_gif, media: status.media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json }) }} | ||
| + %div{ data: { component: 'MediaGallery', props: Oj.dump(height: 343, sensitive: status.sensitive?, 'autoPlayGif': current_account&.user&.setting_auto_play_gif, 'reduceMotion': current_account&.user&.setting_reduce_motion, media: status.media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json }) }} |
nolanlawson commentedOct 14, 2017
This is a partial fix of #5355. It adds a new option to reduce motion, which is disabled by default:
So far, this setting disables these animations:
Here's a before video and after video.
Open questions:
react-motion'sspring()everywhere. But I'm not an expert in React/Redux, so I just implemented it similarly to howautoPlayGifis implemented.