Rename edit-post__fade-in-animation and unify keyframe definitions #65377
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
ciampo
commented
Sep 16, 2024
Comment on lines
-10
to
-12
| // These keyframes should not be part of the _animations.scss mixins file. | ||
| // Because keyframe animations can't be defined as mixins properly, they are duplicated. | ||
| // Since hey are intended only for the editor, we add them here instead. |
There was a problem hiding this comment.
With the changes introduced by this PR, there may be some duplication — but IMO it's a much better compromise than all the potential downsides listed in #64580 (comment)
ciampo
added
[Package] Base styles
This was referenced Sep 16, 2024
|
Size Change: -61 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Sep 17, 2024
|
|
||
| animation: __wp-base-styles-fade-in $speed linear $delay; | ||
| animation-fill-mode: forwards; | ||
| @include reduce-motion("animation"); |
There was a problem hiding this comment.
What are your thoughts on the continued usage of this mixin? For new code, should we just write out the media query like we're doing in the components package?
There was a problem hiding this comment.
I like the approach that we're taking in components package, ie. "only add animation styles if the user does NOT prefer reduced motion".
Having said that, I would prefer not to make such a change here for two reasons:
- consistency: if make such a change, we could consider doing so at the project level?
- the
reduce-motionmixin sets durations to0s, which is technically different from not having an animation at all. For example, I'm not sure how animation and transition events would fire across browsers, and that could introduce unexpected behaviors.
To sum it up: I'd be up for making the change, but I'd make it separately from this PR and gradually:
- refactor usages one by one to
@media not (prefers-reduce-motion) { /* animation code /* }, test for breaking changes - when all usages are migrated, deprecate the mixing
There was a problem hiding this comment.
agree with @ciampo, worth doing but with precaution
There was a problem hiding this comment.
That said, regarding this:
the reduce-motion mixin sets durations to 0s, which is technically different from not having an animation at all. For example, I'm not sure how animation and transition events would fire across browsers, and that could introduce unexpected behaviors
I'm not 100% sure, but I can't see how a duration of 0 would be effectively any different from disabling the animation itself, other than if there's a delay in one of them or something like that which I doubt. Btw, now that I think about it that approach (setting duration to 0) was flawed in that way. A delay would still affect it. One more reason to migrate away from that pattern.
There was a problem hiding this comment.
FWIW the duration is set to 1ms, which may be even more different 😅
This was referenced Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Extracted from #65203
Rename the
edit-post__fade-in-animationto a more generic name (deprecating the previous name), unify underlying keyframe definitions to be under the same package, and add an equivalent "fade out" mixin.Why?
See #64580 (comment)
How?
edit-post__fade-in-animationtoanimation__fade-in, deprecated old nameanimation__fade-outTesting Instructions
Make sure that the
Modalcomponent's enter animation (fade in) works as on trunk, both in Storybook and across various parts of Gutenberg.