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 menu appear animation & start work on handbook #13617

Merged
merged 26 commits into from Feb 7, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Jan 31, 2019

This PR aims to start work on #8029, by creating an animation for use with popovers, and by starting documentation for hwo to best use animation.

It:

  • Adds a "scale and appear" animation to the block toolbar, the more menu, and the block library
  • It adds a very very early draft of an animation document, which includes an early inventory of current animations in use.

GIF:

popover appear

@jasmussen jasmussen self-assigned this Jan 31, 2019

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Jan 31, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 31, 2019

It adds a very very early draft of an animation document, which includes an early inventory of current animations in use.

I don't see this, did I miss something?

@@ -177,6 +177,18 @@ $arrow-size: 8px;
.components-popover:not(.is-mobile):not(.is-middle).is-left & {
margin-right: -24px;
}

// Animate some of the popovers

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

How about we add a prop to the Popover component which would enable a class name which we put here instead?

This comment has been minimized.

@youknowriad

youknowriad Jan 31, 2019

Contributor

Maybe instead, this would be a HoC/renderprop/hook adding the correct className based on the "position" (top left,...)

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

Yep, something like that :)
Whatever is easy to setup and explain in docs. If we ask to add CSS styles to enable animation it might be harder to follow.


.edit-post-more-menu__content & {
transform-origin: top right;
}

This comment has been minimized.

@youknowriad

youknowriad Jan 31, 2019

Contributor

Ideally these styles shoudn't be added here and instead added to each respective component's stylesheet.

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

Or generalized and controlled by the value passed through prop translated into a corresponding class name.

transform-origin: top left;
}

.edit-post-more-menu__content & {

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

Is it something that could be also based on the prop passed to the component?

to {
transform: translateY(0%) scaleY(1) scaleX(1);
}
}

This comment has been minimized.

@youknowriad

youknowriad Jan 31, 2019

Contributor

If this animation is tied to the "popover" component, I guess this style should go there but if it's a very generic animation that can be applied anywhere, maybe we could try working on an "animatation" stylesheet loaded as a dependency where it's being used.

Ideally, SASS would allow these keyframes in mixins in someways, (with rewrite rules maybe) but I'm not sure if that's possible

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Jan 31, 2019

I don't see this, did I miss something?

Ack, looks like I forgot to `git add the new MD file, and now I'm relocating home! I'll be sure to commit it tomorrow, sorry about that.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 31, 2019

I'm looking forward adding some subtle animations to Gutenberg. Thanks for starting this Joen.

@folletto

This comment has been minimized.

Copy link

folletto commented Jan 31, 2019

On principle: good move, great to get this started, nice effect. 🌟

I feel this is the kind of thing I'd push live early right after a release so people can feel it.

Aside: I consider 200ms the upper limit for these animations. So from a theoretical perspective I think we're good.

Have we checked the performance on mobile?

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Jan 31, 2019

FWIW, the animation feels a little slow to me.

@kjellr

This comment has been minimized.

Copy link
Contributor

kjellr commented Jan 31, 2019

Agreed with @melchoyce and @folletto on the speed. I tried speeding it up to 0.01s and it feels pretty good to me. (It could maybe even be a little faster than that, but 0.05s seemed a little too quick).

I did see this error, which doesn't happen to me in master:

inserter

The inserter there pops up, even though there isn't enough space. 🤔

It also seemed weird that the Content Structure and Block Navigation menus didn't have this animation... but I'm guessing those use a different component or something?

animation

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Jan 31, 2019

I experienced the bug that Kjell mentioned as well. And also agree on speeding up the animation a tad. It's looking so beautiful... and FUN!

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

THANK you all for the wonderful feedback! I heartily agree, the animation should be faster, here it is in .1s:

appear

The GIF doesn't capture it, but it is faster.

I also added the documentation file, your thoughts there are welcome.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

The animation uses translate only, so it's really fast, and it seems to work well in the simulator also:

simulator

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

Aside from the the docs and additional feedback, it feels like the primary next step to figure out is the point of origin. It is partially encapsulated in these comments by @gziolo and @youknowriad:

Maybe instead, this would be a HoC/renderprop/hook adding the correct className based on the "position" (top left,...)

Ideally these styles shoudn't be added here and instead added to each respective component's stylesheet.

Or generalized and controlled by the value passed through prop translated into a corresponding class name.

If this animation is tied to the "popover" component, I guess this style should go there but if it's a very generic animation that can be applied anywhere, maybe we could try working on an "animatation" stylesheet loaded as a dependency where it's being used.

These all speak to the same point: this animation needs a point of origin. Which as noted in the docs is a key principle we have to consider — the animation from the ellipsis button and outwards can help create a sense of place and origin of the menu.

There are a few challenges with that, though:

  • These animations have to be RTL aware
  • These animations have to be context aware. For example as illustrated by Kjell, sometimes the popover will upen upwards, and then the transform origin has to change to accommodate that.

If, to @gziolo's suggestion, we could work the transform-origin property into the popover component itself, it seems like we could address all those issues, including CSS context feedback such as #13617 (comment). Can you help here Grzegorz?

jasmussen added some commits Jan 21, 2019

Try menu appear animation & start work on handbook
This PR aims to start work on #8029, by creating an animation for use with popovers, and by starting documentation for hwo to best use animation.

It:

- Adds a "scale and appear" animation to the block toolbar, the more menu, and the block library
- It adds a very very early draft of an animation document, which includes an early inventory of current animations in use.

@jasmussen jasmussen force-pushed the try/menu-appear-animation branch from 58bc526 to 1d1f164 Feb 1, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

I have some time, I'll play with this PR a bit today and see if we can build an animate component or something similar.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

I pushed an update here using a component. The idea is that we'd use this component for several kind of animations.
I also applied the animation automatically to all popovers except the tooltips.
If you all agree with the approach, I think we should document the props of this component instead of documenting the classNames.

Let me know what you think @gziolo @jasmussen

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

This is amazing Riad, thanks so much!

animations

This appears solid to me. Very very nice. Notice how the document outline correctly animates from top center as it should. Solid!

I'm applying the Docs label because to me it seems like the next step is just going over the text a little bit.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

@jasmussen I noticed there's a small issue in this PR (no matter the approach) caused by the fact that the Popover component tries to recompute its position right after it appears depending on its height. (you can see it by try by trying the empty paragraph inserter when the height of the window is small).

Which means if the height is equal to 0 (like when applying the animation), the computation is wrong. This is very specific to the Popover component though and I wonder if I can apply a fix this way:

  • Hide the popover for the first render using CSS visibility that way the height is correct
  • Apply the animation with a very small delay which I think won't be noticeable
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

I fixed the issue mentioned above, for me this is ready. Feel free to take over (README of the component...), if there's need for code tweaks, I'm here to help.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 1, 2019

You do so fast and amazing work Riad, it's a gosh darn privilege to have your help on this.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 1, 2019

I think this PR is very important and open the door to a lot of small animations (using this component) into other places of the UI.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 7, 2019

@adamsilverstein I'm trying to take a similar approach to your PR here to try to fix the tests.

The difference is I named the delay function, "waitForAnimation" to clarify that it's not something we should use everywhere but explicitly when there are animations ongoing. Using delays randomly can hide real issues in e2e tests.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 7, 2019

I did some tweaks to the e2e tests, I'm still evaluating if there are other places where I missed the waitForAnimation call. I think it's not a perfect solution but it's an ok one for now. We shouldn't block the PR because we don't have a perfect solution for e2e tests. the important thing is that the e2e tests pass consistently.

On the docs. I pushed tweaks to the Animate README. I'd personally prefer if we remove the animations doc entirely or at least rewrite it to link to the README of the Animate component instead. I don't want these classnames to be documented, that way people won't start using them in plugins...

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 7, 2019

I'd personally prefer if we remove the animations doc entirely or at least rewrite it to link to the README of the Animate component instead.

Do it! The argument is solid and now that we have a component for it, we have a path forward to making the animations reusable in a better way.

@jasmussen jasmussen requested a review from notnownikki as a code owner Feb 7, 2019

@youknowriad
Copy link
Contributor

youknowriad left a comment

LGTM 👍

Another review would be good as I worked a lot on this PR.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 7, 2019

You're the greatest. All of you.

gziolo added some commits Feb 7, 2019

@gziolo
Copy link
Member

gziolo left a comment

I applied two small tweaks to docs myself.

There is one use case where the usage of Animate might be problematic.

--- | --- | --- | ---
`type` | `string` | `undefined` | Type of the animation to use.
`options` | `object` | `{}` | Options of the chosen animation.
`children` | `function` | `undefined` | A callback receiving a list of props ( `className` ) to apply to the DOM element to animate.

This comment has been minimized.

@gziolo

gziolo Feb 7, 2019

Member

When children isn't a function it will explode at the moment.

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

I think it's fine, it's a requirement for the component. it's like saying the options is an object and if you pass a number it will explode.

I do see the specificity in JSX about the children prop but ultimately I think we can just consider it a requirement to be a function.

This comment has been minimized.

@gziolo

gziolo Feb 7, 2019

Member

I think it's a conceptual issue with render props :)

*/
import classnames from 'classnames';

function Animate( { type, options = {}, children } ) {

This comment has been minimized.

@gziolo

gziolo Feb 7, 2019

Member

Should it exit early when children isn't a function?

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

Same as I was saying above, we don't guard about the other props. I'm hesitant to add a special case for children.

@@ -0,0 +1,10 @@
/**

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

Understanding the desire that this is introduced to help unblock the pull request, I hesitate on whether this should be considered an "unstable" API or not. I think as proposed it's at least doing well enough to not assume that it operates on some timer, since I think an implementation leveraging transitionend is probably a worthwhile exploration for further improvement. But the bigger issue to me is forcing it to be a conscious part of someone's mind in authoring tests, and whether that's strictly necessary. It seems far too prone to being forgotten and causing unexpected test failures, though if they're obvious enough and consistent failures, perhaps they're not truly "unexpected" and it's okay.

In short, I wonder if it would be a good idea to mark this as unstable, or perhaps not if we think that even if it be part of a public API (and usable directly) that we could find some other way to "automate" this on behalf of a test author in the future.

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

Other possibilities to avoid it being a conscious part of the developer test authoring workflow is to ensure that most of the interactions which involve an animation are sufficiently abstracted. So rather than a test including:

await page.click( 'button[aria-label="More options"]' );
await waitForAnimation();

We'd see something like await openMoreOptions(); which handles the animation implementation detail internally.

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

I do agree we should "automate" this on behalf of the test author behind utilities like:

  • insert block
  • click menu item
  • switch to editor mode
    ...

at that point we could hide it entirely from the public API

A lot of tests need to be updated though, and I don't think that's the PR for it.

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

@aduth Looking at the tests, I don't think we can entirely abstract all the interactions without creating very specific functions. One example that comes to mind, is do we want to add an openFontSizePicker utility where it's very specific to a given a block in a given state?

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

A lot of tests need to be updated though, and I don't think that's the PR for it.

As long as there's some reasonably clear path forward (and with at least a nod to the fact we're adding technical debt) I'm fine to not let it block the work here. It would be good to have an issue describing the future planned work.

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

One example that comes to mind, is do we want to add an openFontSizePicker utility where it's very specific to a given a block in a given state?

No, probably not. But then it resurfaces the problem of imposing the need to be aware of a delay to the test author. My original intent with raising the point was in trying to see through the implications, and if not to solve it in place, have some idea for a future goal. To me, that future goal ideally wouldn't involve waitForAnimation existing, hence why I'd considered whether to mark it as unstable.

The alternatives I'd have in mind which bypass the need for the function are rough in my mind, and in many ways similarly flawed / technically challenging:

  • Not have the transitions exist for the tests. It's still an issue that these are going to add measurable delays to the runtime of every build.
  • In some way, every transitionstart which ever occurs on a page is monitored such that when a test includes an interaction (e.g. click) which either triggers / follows (unsure which yet) a transition, the following action would be delayed automatically.

I'm also open to saying that accepting that this needs to be consciously considered as part of writing an end-to-end tests. I'm sure there's an argument that these "steps" imitate a user's physical actions, which includes that when a user clicks a button, they may have to sit and wait for the animation.

tl;dr: I have concerns, and I wanted them to be noted, but I also think this is good enough in its current state to be able to move forward on.

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

Thanks for the thoughts, I'm personally not concerned about stable/unstable API since it's just a test utility that said, I'll follow up with an issue with these great thoughts.

I just had an idea probably similar to your first point here: maybe in the tests we load a specific stylesheet forcing all animations to be very quick (unnoticeable by tests) like 1ms.

  • It ensures we're testing the existence of the animation (like prod)
  • It ensures the tests are stable.

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

I'm personally not concerned about stable/unstable API since it's just a test utility

Speaking of breaking changes which would require a major version bump to @wordpress/e2e-test-utils: We should include a note (enhancement, minor bump) for the addition of waitForAnimation and ideally document it somewhere if we're doing that for these utilities (I'd suspect not).

This comment has been minimized.

@youknowriad

youknowriad Feb 7, 2019

Contributor

Actually that package hasn't been released yet. It feels weird to add just waitForAnimation to the changelog where it only includes a single entry Initial release.

Good call though, I added the Animate component to the components changelog.

This comment has been minimized.

@aduth

aduth Feb 7, 2019

Member

It feels weird to add just waitForAnimation to the changelog where it only includes a single entry Initial release.

Oh, I didn't realize it hadn't been released yet. It's fine to just leave it then as "Initial release".

@gziolo

gziolo approved these changes Feb 7, 2019

Copy link
Member

gziolo left a comment

I get your explanations, it isn't perfect but this is what it is in every other place we use render props.

Feel free to merge when documentation is properly linked and suggestion from @aduth is addressed.

youknowriad added some commits Feb 7, 2019

@youknowriad youknowriad merged commit 3f0ef0b into master Feb 7, 2019

1 check passed

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

@youknowriad youknowriad deleted the try/menu-appear-animation branch Feb 7, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 7, 2019

Let's animate this editor

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