-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Code Quality: Improve prefix, better scope, for animations #11752
Conversation
I'd personally suggest removing this file for the auto-imported files, it doesn't make sense to include in a lot of generated stylesheets. Let's make it an explicit import. |
Edited PR, I don't think this will fix issue #11673. |
assets/stylesheets/_animations.scss
Outdated
|
||
@mixin animate_fade { | ||
animation: animate_fade 0.1s ease-out; | ||
@mixin editor-animation__animate-fade { |
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.
This doesn't seem to be used, I'll just remove
assets/stylesheets/_animations.scss
Outdated
animation-fill-mode: forwards; | ||
} | ||
|
||
@mixin move_background { | ||
@mixin editor-animation__move-background { |
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.
This doesn't seem to be used either, I'd consider removing too.
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.
There's this test for this specific animation:
it( 'should ignore keyframes', () => {
const callback = wrap( '.my-namespace' );
const input = `
@keyframes editor-animation__move-background {
from {
background-position: 0 0;
}
}`;
const output = traverse( input, callback );
expect( output ).toMatchSnapshot();
} );
I agree it doesn't seem to be used, but I'm also not quite sure either what this test does or what to do with it. Any suggestions?
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.
you can just replace it with any animation (defined or not)
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.
To clarify, it's only testing that this editor styles wrapping behavior doesn't touch the animations.
assets/stylesheets/_animations.scss
Outdated
} | ||
|
||
@mixin slide_in_right { | ||
@mixin editor-animation__slide-in-right { |
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.
This is used only once, I'd consider moving it to the place it's used :)
assets/stylesheets/_animations.scss
Outdated
} | ||
|
||
@mixin slide_in_top { | ||
@mixin editor-animation__slide-in-top { |
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.
Also used once.
assets/stylesheets/_animations.scss
Outdated
|
||
@mixin region_focus($speed: 0.2s) { | ||
animation: editor_region_focus $speed ease-out; | ||
@mixin editor-animation__region-focus($speed: 0.2s) { |
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.
Also used once, and seem very specific to region focus, so just move it there.
assets/stylesheets/_animations.scss
Outdated
to { | ||
margin-top: 0; | ||
} | ||
@mixin editor-animation__rotation($speed: 1s) { |
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.
Also used once.
Looking very good, also agree with all of @youknowriad's comments, removing a lot of these would make everything smaller :) Mostly happy to see that if you work through his comments I think we're only left with 3 |
Good feedback, will address all points in a bit.
I tested reloading that specific CSS file in a separate tab to verify this worked, and yes indeed moving the keyframe rules from the mixins file removes them from the block-library/style.css file. One note for perhaps a separate ticket — |
* Make cssnano remove all comments As referenced by @jasmussen [here](#11752 (comment)), this should fix the fact that `/*` style comments are not removed by cssnano. * Linting.
This PR maybe fixes #11673, maybe fixes #11668. At least, this is the intention. It does two things: - It changes the naming scheme for all animations. Instead of having a mix of underscores and dashes for word separation, it uses BEM (to the best of my ability) to add consistency and a new naming convention for all animations. - It adds a prefix to all animations, indicating they are for the editor. Though perhaps we should have a more _admin_ specific prefix given these might one day be used outside the admin? - It moves all keyframe animations out of the mixins file, and into the edit-post style. This is because keyframe definitions don't work well with mixins and are just duplicated. This PR has been tested for all the animations I could find that were using the ones defined. But please give me a sanity check. By the way I noticed the following two animations do not appear to be used: - editor-animation__animate-fade - editor-animation__move-background Should we remove those? Or can anyone recall where they were intended to be used?
This retires a few animations, and moves others that are only used once to their respective scopes.
Gallery related, in #11668 (comment).
a0737cb
to
8f232ce
Compare
Okay, I think I addressed everything. |
👍 except for the failing build ;) |
Restarting, hoping it passes this time :D There's been some travis shenanigans lately. |
@@ -19,6 +19,15 @@ | |||
height: 4px; | |||
border-radius: 100%; | |||
transform-origin: 6px 6px; | |||
@include animate_rotation; | |||
animation: editor-animation__rotation 1s infinite linear; |
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.
Could we give it a name using our guidelines: components-spinner__animation
?
} | ||
} | ||
|
||
@keyframes editor-animation__slide-in-top { |
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.
could this be edit-post-fullscreen-mode__slide-in-animation
?
|
||
body.is-fullscreen-mode & { | ||
top: 0; | ||
} | ||
} | ||
} | ||
|
||
@keyframes editor-animation__slide-in-right { |
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.
This could be edit-post-layout__slide-in-animation
packages/edit-post/src/style.scss
Outdated
// 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 editor-animation__loading-fade { |
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 more edit-post__loading-fade-animation
assets/stylesheets/_animations.scss
Outdated
|
||
@mixin modal_appear() { | ||
animation: modal-appear 0.1s ease-out; | ||
@mixin editor-animation__modal-appear() { |
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.
This mixin doesn't seem useful as use only once, we can move it and rename it as well?
Okay I think I got all those fixed. Final look? |
😍 thanks @jasmussen |
} | ||
} | ||
|
||
@keyframes edit-post__modal-appear-animation { |
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.
haha, almost :P This should be components-modal__appear-animation
:)
"Root Package" - "last folder" __ "some name"
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.
Haha, it'll stick on my brain any day now! Thank you.
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.
LGTM 👍 Thanks for the updates @jasmussen Feel free to merge after the last minor comment.
This PR maybe fixes #11668. At least, this is the intention. It does two things:
This PR has been tested for all the animations I could find that were using the ones defined. But please give me a sanity check.
By the way I noticed the following two animations do not appear to be used:
editor-animation__animate-fade
editor-animation__move-background
Should we remove those? Or can anyone recall where they were intended to be used?
Another question, right now these animations are prefixed
editor-animation
. Should we find a prefix that indicates these might be used outside of the editor, in the rest of the admin, one day?Finally — what is the risk associated with changing these animation names? Do we have any way to know if plugin developers might have used these?