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.

Sign up for GitHub

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

Jump to bottom

Rename edit-post__fade-in-animation and unify keyframe definitions #65377

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Rename edit-post__fade-in-animation and unify keyframe definitions #65377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00c2224
Remove duplicate keyframe definition across packages
ciampo Sep 10, 2024
378e4c9
Deprecate edit-post__fade-in-animation, replace with animation__fade-in
ciampo Sep 10, 2024
d1693aa
Add animation__fade-out mixin
ciampo Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions packages/base-styles/_animations.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,37 @@
@mixin edit-post__fade-in-animation($speed: 0.08s, $delay: 0s) {
animation: edit-post__fade-in-animation $speed linear $delay;
@mixin keyframes($name) {
@keyframes #{$name} {
@content;
}
}

@mixin animation__fade-in($speed: 0.08s, $delay: 0s) {
@include keyframes(__wp-base-styles-fade-in) {
from {
opacity: 0;
}
to {
opacity: 1;
}
}


animation: __wp-base-styles-fade-in $speed linear $delay;
animation-fill-mode: forwards;
@include reduce-motion("animation");
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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-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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @ciampo, worth doing but with precaution

Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW the duration is set to 1ms, which may be even more different 😅


@mixin animation__fade-out($speed: 0.08s, $delay: 0s) {
@include keyframes(__wp-base-styles-fade-out) {
from {
opacity: 1;
}
to {
opacity: 0;
}
}


animation: __wp-base-styles-fade-out $speed linear $delay;
animation-fill-mode: forwards;
@include reduce-motion("animation");
}
Expand All @@ -8,3 +40,9 @@
transition: all 0.5s cubic-bezier(0.65, 0, 0.45, 1);
@include reduce-motion("transition");
}

// Deprecated
@mixin edit-post__fade-in-animation($speed: 0.08s, $delay: 0s) {
@warn "The `edit-post__fade-in-animation` mixin is deprecated. Use `animation__fade-in` instead.";
@include animation__fade-in($speed, $delay);
}
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/list-view/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@

&.is-visible .block-editor-list-view-block-contents {
opacity: 1;
@include edit-post__fade-in-animation;
@include animation__fade-in;
}

.block-editor-block-icon {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/utils/test/transform-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ describe( 'transformStyles', () => {

it( 'should ignore keyframes', () => {
const input = `
@keyframes edit-post__fade-in-animation {
@keyframes __wp-base-styles-fade-in {
from {
opacity: 0;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
background-color: rgba($black, 0.35);
z-index: z-index(".components-modal__screen-overlay");
display: flex;
// This animates the appearance of the white background.
@include edit-post__fade-in-animation();
// This animates the appearance of the backdrop.
@include animation__fade-in();
}

// The modal window element.
Expand Down
16 changes: 0 additions & 16 deletions packages/edit-post/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,6 @@
@import "./components/meta-boxes/meta-boxes-area/style.scss";
@import "./components/welcome-guide/style.scss";

/**
* Animations
*/

// 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.
Comment on lines -10 to -12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

@keyframes edit-post__fade-in-animation {
from {
opacity: 0;
}
to {
opacity: 1;
}
}

body.js.block-editor-page {
@include wp-admin-reset( ".block-editor" );
}
Expand Down
16 changes: 0 additions & 16 deletions packages/edit-site/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,20 +91,4 @@ body.js.site-editor-php {
}
}

/**
* Animations
*/

// 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 they are intended only for the editor, we add them here instead.
@keyframes edit-post__fade-in-animation {
from {
opacity: 0;
}
to {
opacity: 1;
}
}

@include wordpress-admin-schemes();
16 changes: 0 additions & 16 deletions storybook/stories/playground/fullpage/style.lazy.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,3 @@
display: block;
}
}

/**
* Animations
*/

// 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.
@keyframes edit-post__fade-in-animation {
from {
opacity: 0;
}
to {
opacity: 1;
}
}
Loading