-
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
Edit site: Restore animations on site hub and canvas #62386
Conversation
Size Change: +77 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
a858775
to
3b536af
Compare
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. |
72c8e06
to
5e42f2d
Compare
For me as it stands, this PR is a step in the wrong direction. I understand that it restores a better animation but it compromises something that I feel is fundamental when we start thinking about the site editor shell. This introduces a strong dependency between the site hub that is rendered in the site editor shell and the frame. It assumes that there's an "editor" that is present in the site editor. While it's the case now, there might be pages where there's no block editor. Like the "media library" for instance (which is going to reuse the site editor layout/shell). So IMO, there are two paths forward:
I noticed that the site hub is now always visible in distraction free mode in this PR. |
Thanks for sweating the details! Between trunk and this PR, I think both have room for improvement. Technical discussions aside—though I think those are important to address—there's something about the animation orchestration that's still not quite right. First off it'll help to think of the default site logo, the WordPress logo, as being the same as an actual site logo. In the latter case, we need to solve #49501 so site logos that are transparent have a background. Once that's done, we can think of both the W and the site logo, as simply icons on a billboard. For the WordPress logo, the billboard is a dark color, similar to the default WordPress background. For a site icon, it's a site-background-colored billboard. In both cases, they stack like this: I find when orchestrating animation it often helps to think of each element as a physical card on a table. When it animates, it has to come from somewhere, have a stacking order, physical properties, etc. That places the site icon as the top level item, above both site and edit views. And the billboard behavior will help explain what happens when we move into the editor. For me that puts the ideal animation somewhere between trunk and this PR. |
Interesting (looking at the screenshot). That's not supposed to happen even in trunk and I'm not sure that's related to the site hub at all. I'm on WCEU these days so I might not have the focus time to dive deeper her in the next coming days, but let's try to work on these details, and find a path forward for these animations... I'll probably have better availabilities next week. |
😃 I guess details are relative. I didn’t consider the broken bits of the animation in trunk as details. I guess they are but less so than some timing/orchestration concerns. I feel it’s a bit of a euphemism to have labeled this an "enhancement". I’d agree there is room for improvement. I may be able to deduce what that might mean for you having looked at the timing differences with trunk and this branch. Mainly I see the site icon waits before scaling up until the canvas frame has gotten into fullscreen placement. I suspect that came about due to a technical rather than design concern in #61579 but if it’s considered better this branch can certainly follow suit.
Well for a bit I’d begun to think I had some funny version of Chrome or something but I've since tested other versions and computers. Now it can easily be tested anywhere cause it’s in the WP nightly. |
Just to clarify, I consider details to be the individual pieces that make up the whole. The whole is just a collection of details, and if any one of them is off, the whole thing feels off. So I mean it as the sincerest form of a compliment, that you care about the details. |
What
Restores animations and another detail or two that were lost in #61579.
Why
The previous transition was a good bit nicer. The current will lend to an impression that the app is shoddy.
More
The idea with #61579 was to simplify. It certainly had a nice ratio of deletions to additions but while some concerns may have been simplified others were complicated. Moreover, its deletions must not have been enabled just because the hub is no longer persistent and fixed above everything. I say that because this PR has net zero additions while restoring the previous transition.
The main caveat with this is that Distraction Free mode has a visual difference. The hub persists there too. However, the mode works. To me, this makes a better trade-off and it can be completely fixed too. I don’t want this PR getting any more unwieldy for review so I’ll put such changes in another.
How?
The rest of these are more independent and could probably each be extracted as their own PRs in case any of them prove debatable.
Testing Instructions
Screenshots or screencast
On this branch (0.2x speed)
restored-0.2x.mp4
On trunk (0.2x speed)
trunk-0.2x.mp4