-
Notifications
You must be signed in to change notification settings - Fork 4.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
Components: Deprecate reduceMotion
util
#60839
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: 0 B Total Size: 1.75 MB ℹ️ View Unchanged
|
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.
Makes sense 👍
Thank you, @mirka!
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.
Good call 👍
@@ -3,6 +3,10 @@ | |||
* | |||
* @param {'transition' | 'animation' | string} [prop='transition'] CSS Property name | |||
* @return {string} Generated CSS code for the reduced style | |||
* | |||
* @deprecated Write your own media query instead, |
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.
Not in this PR, but should we also refactor the existing usages? The best way to ensure that there will be new usage of this deprecated utility is to keep using it in the Gutenberg codebase.
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.
+1 ^
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.
It's just a matter of prioritization. Ideally they should be rewritten, but this one is pretty benign compared to the million other things we need to rewrite 😭 Though, it's probably a good first issue for new contributors so I'll post an issue.
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.
No urgency here, an issue sounds good 👍
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.
Very nice! Only a small question.
@@ -3,6 +3,10 @@ | |||
* | |||
* @param {'transition' | 'animation' | string} [prop='transition'] CSS Property name | |||
* @return {string} Generated CSS code for the reduced style | |||
* | |||
* @deprecated Write your own media query instead, | |||
* e.g. `@media not ( prefers-reduced-motion ) { ...some animation... }` or |
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.
Maybe this is just a typo or maybe it's another valid way to implement this, but:
-@media not ( prefers-reduced-motion ) {
+@media not ( prefers-reduced-motion: reduce ) {
Are we sure those will behave the same way with no weird edge cases? Is there any practical difference between these two that could be relevant?
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.
The current docs do say they should be equivalent. They would differ if they ever added another possible value for prefers-reduced-motion
, for example reduce-somewhat
or absolutely-no-motion-im-serious
, but in that case I think it'd be better for those values to be included in our simple booleans by default.
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.
The linked spec section is a very interesting read, and makes a lot of sense 👍
What?
Deprecates the internal
reduceMotion
utility function in the components package.Why?
First discussed in #60560 (comment).
@media not ( prefers-reduced-motion )
block.It isn't particularly important to remove the existing usages right now — they can be rewritten as needed. We'll be able to remove this util from the codebase once they're all rewritten though.
Testing Instructions
In your IDE, existing usages of this util should show that it is deprecated.