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

Try: Add animation to sidebar. #13635

Merged
merged 4 commits into from Feb 12, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Feb 1, 2019

This is another "Try" branch for an animation. This animates in the sidebar. See also #13617

sidebar animation

What do you think? This one I feel less certain about.

@jasmussen jasmussen self-assigned this Feb 1, 2019

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Feb 1, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

This could use the same component from the other PR. what do you think?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

If we dig it, absolutely it should! I'll rebase and make it happen if/when the other one ships.

I'll also update the Publish dialog, which uses the same animation.

@kjellr

This comment has been minimized.

Copy link
Contributor

kjellr commented Feb 1, 2019

This is great. Definitely an improvement.

It seems a little weird to me that while the sidebar slides in, the content next to it abruptly pops over. But I figure it'd be fairly un-performant to animate all that content, too. 😄 The action here is all about the sidebar, so I think it's fine as is.

I am seeing one bug though — the menu isn't appearing at all for me on smaller breakpoints:

sidebar

@mapk
Copy link
Contributor

mapk left a comment

Really nice animation! Can't wait to get this in. I did experience the same issue Kjell found for the smaller screens as well though. After that bug fix, LGTM.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 6, 2019

Just as an update, I'm waiting for #13617 to land before rebasing and fixing up this one.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 8, 2019

Can we rebase this and try the "Animate" component?

@jasmussen jasmussen force-pushed the try/sidebar-animation branch from 4217fd3 to e69bad9 Feb 8, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 8, 2019

Rebased this, but I won't have time to look into the animate component today as I have some other things I need to wrap. But if you have a bunch of time, feel free to commandeer this PR 👍 👍

@gziolo
Copy link
Member

gziolo left a comment

I wanted to double check whether we can remove some code which might be now handled inside Animate component.


body.is-fullscreen-mode & {
top: 0;
}
}
}

@keyframes edit-post-layout__slide-in-animation {
@keyframes edit-post-post-publish-panel__slide-in-animation {

This comment has been minimized.

@gziolo

gziolo Feb 8, 2019

Member

It's exactly the same as @keyframes components-animate__slide-in-animation {. Is it still necessary?

@@ -173,15 +173,15 @@
width: $sidebar-width;
border-left: $border-width solid $light-gray-500;
transform: translateX(+100%);
animation: edit-post-layout__slide-in-animation 0.1s forwards;
animation: edit-post-post-publish-panel__slide-in-animation 0.1s forwards;

This comment has been minimized.

@gziolo

gziolo Feb 8, 2019

Member

How does it differ from .components-animate__slide-in?

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Contributor

Yes, it's not ideal, but since the animation is not inheritant to the panel itself but more to the fact that it's used as a sidebar by the edit-post module, it doesn't make sense to introduce the component inside the panel, which makes this issue harder to solve. I decided to keep the CSS for this particular panel for now.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 8, 2019

Travis seems to be very unhappy about this animation. It looks like we need to land #13769 first before we can proceed. In the meantime, this could use a review from someone who can UI part of the PR. Code wise it looks good. Nice work @jasmussen and @youknowriad. I really like this new Animate component and the way how it can simplify usage and unify behaviors for future work.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 8, 2019

This might need some tweaks for RTL, I need to check.

@youknowriad youknowriad force-pushed the try/sidebar-animation branch from e871f41 to 01456b2 Feb 8, 2019

@youknowriad youknowriad force-pushed the try/sidebar-animation branch 2 times, most recently from ee53e0e to 515bbd5 Feb 8, 2019

@jasmussen jasmussen requested review from nerrad and ntwb as code owners Feb 11, 2019

This seems fixed to me now.

@youknowriad
Copy link
Contributor

youknowriad left a comment

LGTM 👍 But I worked on the PR, so another approval would be good.

@youknowriad youknowriad force-pushed the try/sidebar-animation branch from 1e2fce1 to 480b977 Feb 12, 2019

@youknowriad youknowriad merged commit 9dc1a33 into master Feb 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Phase 2 automation moved this from Tighten Up to Done Feb 12, 2019

@youknowriad youknowriad deleted the try/sidebar-animation branch Feb 12, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 12, 2019

HAH you just merged :D :D — good, because I was about to 👍 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 12, 2019

I had some additional enhancements but they are a bit sensitive so I decided to make a separate PR for them. Thanks so much for your help, Riad!

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Feb 16, 2019

Looking a bit into this, I've noticed a couple things:

The sidebar animation happens also on page load:

  • edit an existing post or create a new one
  • if the sidebar is set to be open, the animation runs on page load (also when you refresh the page)

Is this intentional? Doesn't seem ideal to me: I don't see why users should see an animation every time they open the edit post page. Note: for some reason, this is more evident in Firefox and Safari, especially when the post has some long content.

More importantly: on Windows the vertical scrollbars are always visible. Whether users open the edit post page or toggle the sidebar, the visual effect is not so great:

  • test with a post with some long content (so there's a vertical scrollbar)
  • toggle the sidebar to make it appear
  • the vertical scrollbar appears immediately, delimiting the sidebar area
  • the sidebar area is not fully taken by the sidebar content yet, which is being animated

Basically, for a fraction of a second users see an empty column with a vertical scrollbar. I've tried to make an animated GIF to better illustrate, but some frames get removed and it doesn't give a clear idea of the actual visual effect, which is not nice to see.

/Cc @jasmussen @youknowriad

@kjellr

This comment has been minimized.

Copy link
Contributor

kjellr commented Feb 18, 2019

Thanks, @afercia!

The sidebar animation happens also on page load:

Yeah, this is also the case for our fullscreen load animation. I wonder if there's a way we can assign these animation classes only after the page's been fully loaded... 🤔

I've opened #13925 to track this.

Basically, for a fraction of a second users see an empty column with a vertical scrollbar. I've tried to make an animated GIF to better illustrate, but some frames get removed and it doesn't give a clear idea of the actual visual effect, which is not nice to see.

This happens on Mac too, if you have scrollbars enabled:

reload

(I slowed down the animation quite a bit to make sure it got picked up by the GIF, but this gets the general idea across)

I'm not sure if we can do anything about this, but I've opened #13926 to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment