Editor: Improve Header layout #62636
Conversation
|
Size Change: +455 B (+0.03%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
stokesman
added
[Type] Enhancement
stokesman
force-pushed
the
update/editor-header-layout
branch
from
9eaadad
to
f00c36b
Compare
|
Flaky tests detected in f00c36b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9568455631
|
stokesman
changed the title
| function CollapsableBlockToolbar( { isCollapsed, onToggle } ) { | ||
| export default function CollapsibleBlockToolbar( { isCollapsed, onToggle } ) { |
There was a problem hiding this comment.
Typo correction, the filename is correct but the component itself was typo’d.
packages/editor/src/components/collapsible-block-toolbar/index.js
Outdated
Show resolved
Hide resolved
| // Some plugins add buttons here despite best practices. | ||
| // Push them a bit rightwards to fit the top toolbar. | ||
| margin-right: $grid-unit-10; | ||
| // Some plugins add buttons here despite best practices — accommodate them with a gap. | ||
| &:not(:last-child) { | ||
| margin-inline-end: $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
I thought conditionally adding this "gap" was better than universally doing so. It saves a bit of space when plugins aren’t adding buttons there (or "Top toolbar" is not on as it also serves there). Note this change would leave no space before the collapsible block toolbar’s toggle button so that is instead handled by header styles.
|
|
||
| @include break-large() { | ||
| width: min(100%, 450px); | ||
| } |
There was a problem hiding this comment.
I'm not sure just why the width needed a distinct maximum at the large breakpoint. I moved the 450px up to the base style on L13 and with the overall changes the Document Bar smoothly narrows according to available space.
packages/editor/src/components/collapsible-block-toolbar/style.scss
Outdated
Show resolved
Hide resolved
| .editor-collapsible-block-toolbar__toggle { | ||
| margin-left: 2px; // Allow focus ring to be fully visible | ||
| } |
There was a problem hiding this comment.
The parent element’s style changes now ensure this isn’t necessary.
|
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. |
|
Nice one, another very important detail to get right. Appreciate the PR. At a glance, works okay. I know this is something both @richtabor and @jameskoster have tinkered with, so here's a CC. Before:
Note how the title is visible on mobile, but not in a functional way. And note how it is flush with the save button as well, on the tablet breakpoint. After:
Note how both the mentioned issues are addressed, the title is hidden on mobile, and the overall responsiveness is more fluid. So how do you see or change the document title on mobile? Through the document inspector as part of the new unified summary panel:
At this point, I would say this more or less fixes #52032. If there are a couple of small bits from that issue you can absorb, feel free to, though! |
|
Here’s something that may need addressed at some point. The site icon in the header currently is rendered into a slot. Conceivably, there could be an app that chooses not to fill the slot yet end up with a space for it anyway because the styles in this branch don’t account for that. It seems a moot point for now because these components are private APIs and all apps are filling the slot. Still, in trunk it happens that the space is not reserved in such cases: This branch without a fill for the slot: Trunk without a fill for the slot: |
stokesman
force-pushed
the
update/editor-header-layout
branch
from
914001f
to
132045e
Compare
|
This is looking good, though I'm a little anxious about hiding the title on mobile since it removes access to the Command Palette. I've seen comments elsewhere about revealing the undo/redo buttons on mobile too. I wonder if we could hide more, and surface actions through the command palette instead. 
Probably unrelated, but potentially worth fixing here; I see a 'Preview' button on mobile, which is duplicative of the external icon button: 
|
I wanted to keep it around at mobile. There’s enough room for it to be functional (providing we resolve that odd "Preview" button). I wanted to hide at some point to help preserve just how narrow the header can currently go but I didn’t see another breakpoint at < mobile that could be used for that though. Perhaps it doesn’t have to hide and can just squish out of existence like it currently does.
I think it’s a worthwhile exploration though perhaps not so much for this PR.
Thanks for pointing that out. I’ve been seeing it too and wondered why Joen’s screen recording didn’t have it. Then while testing most recently I noticed it was absent during one session but seems like a refresh later it was back. I haven’t yet looked into it. It does seem unrelated but I'll try and understand what’s happening.
After making that comment I realized this was busted in the post editor when the editor isn’t fullscreen (also at less than 782px where there is no fullscreen) as it hides the Site icon. It’s effectively the same issue I referred to in the comment although the slot is filled and its button is hidden by CSS. I suppose I could have written some style overrides only for the post editor but I decided it’s probably neater overall to address it in the header itself. So I added 07dd535 and 132045e to do so. The post editor won’t fill the slot unless fullscreen and the header is aware of whether the slot is filled or not. |
stokesman
force-pushed
the
update/editor-header-layout
branch
from
132045e
to
30d5b4a
Compare
I looked into this some and that button is only present when editing a post type that can be previewed (i.e. pages and not template parts, patterns or others). Then it’s hidden at < small breakpoints. As best I can tell this came about in #56921 diff of relevant addition and seems like a mistake. Shouldn’t the button be hidden for smaller viewports instead of shown? I've pushed a commit 41ba177 that makes it so. It’s an aside but the Preview button is not totally duplicative of the View button because the former will show the post as edited and the latter will show the post as saved. I wonder if it’d be an improvement to combine the controls into a menu (for the post types it’s applicable to anyway). More aside; here are some other details I noticed that seem like they warrant fixes but are not related enough to this PR:
I've gone ahead and tried keeping it around for about 100px less than mobile 30d5b4a. So it’s now closer to how it is in trunk. Here’s the max-width it’s still shown at when editing a page: And when editing a template: If we wanted, a separate breakpoint could be used to keep the Document Bar visible even narrower for templates but I'm not sure it’s worth it. |
|
Ah, I guess it's because the Preview action is bundled in the device preview toggles on desktop. Obviously these aren't applicable on smaller screens, hence they get replaced with the 'Preview' text button. The whole viewing/previewing menu needs some attention – obviously that's a separate endeavour.
I think that looks okay, and leaves the door open to something like #62636 (comment) down the road. |
stokesman
force-pushed
the
update/editor-header-layout
branch
from
30d5b4a
to
deaba73
Compare
Right, thank you for reminding me. So the minor issue there is is that currently the separate Preview button isn’t shown at the same breakpoint that the Device Preview dropdown is hidden. I've reverted the change I made regarding that and because that affects the space available for the Document Bar (while editing a page) revised the custom breakpoint used to hide it. It’s now at 403px instead of 382px. Here are the latest page/template captures for the max-width it’s still present. Editing a page (the Document Bar is still present but totally obscured): Editing a template: |
|
Obviously the mobile experience is not perfect, but the enhancements here are a step in the right direction compared to trunk. I think this one just needs a code review. |
|
@stokesman, do you mind rebasing this on top of the latest trunk? I don't like some metric changes ( cc @jeryj |
stokesman
force-pushed
the
update/editor-header-layout
branch
from
deaba73
to
7238351
Compare
I appreciate you pointing that out. Looks like this time around |
|
I've now reverted the conditional rendering changes to see how that’d affect the metrics. Here’s the run after. It helped with Anyway, I think it’s fine to leave those out and focus the PR on the the layout improvements. CSS could be used to achieve what the conditional rendering changes were doing and I’ll look to try that separately. |
stokesman
changed the title
stokesman
force-pushed
the
update/editor-header-layout
branch
from
c36397d
to
6406918
Compare
| const hasFills = Boolean( fills && fills.length ); | ||
|
|
||
| if ( ! hasFills ) { | ||
| return children; | ||
| } |
There was a problem hiding this comment.
@stokesman Is this change required? If yes, can you elaborate more on "why"? Thanks.
There was a problem hiding this comment.
I didn’t have to remove this but it seemed redundant if we let the Header decide whether to render the slot. "Why" is hopefully clarified in my reply to your other comment on the header.
There was a problem hiding this comment.
Thanks for the explanation, @stokesman! I guess it's okay to change the component's behavior; it's a private component, so there are no requirements for BC.
| { hasBackButton && ( | ||
| <motion.div | ||
| variants={ backButtonVariations } | ||
| transition={ { type: 'tween' } } | ||
| > | ||
| <BackButton.Slot /> | ||
| </motion.div> | ||
| ) } |
There was a problem hiding this comment.
Will there be a case when a "back button" is missing? Seems like a problematic path for fullscreen users.
Cc @youknowriad (since you worked on this unification recently).
There was a problem hiding this comment.
I don't understand the question. Would you mind clarifying?
There was a problem hiding this comment.
I think the "back button" is always present. It can be overridden by third-parties though.
There was a problem hiding this comment.
I think the "back button" is always present. It can be overridden by third-parties though.
Exactly. The header should always render a "back button", either the default or one provided by third-parties. This condition and the BackButton slot-fill changes seem unnecessary.
There was a problem hiding this comment.
Will there be a case when a "back button" is missing?
In the Post editor the back button is missing when not fullscreen (and at < 783px even with fullscreen preference on). Currently it’s done via CSS. The header layout needs to account for that and rather then add overriding styles in the edit-post package, I thought these changes would be cleaner overall and support the conceivable use case for a third-party to not render a back button (without them having to add overriding styles to avoid empty space). I’d noted this first in #62636 (comment) and later mentioned why I opted for these changes #62636 (comment) (in the latter part, after the hr).
| <div | ||
| className={ clsx( 'editor-header edit-post-header', { | ||
| 'has-back-button': hasBackButton, | ||
| 'has-center': hasCenter, |
There was a problem hiding this comment.
Do we really need this kind of classes? Can't we figure out a unified layout that works without requiring these?
There was a problem hiding this comment.
Potentially more advanced and probably less readable selectors in the CSS could be used. I have another design change I’d like to propose for the header that should eliminate the need for has-center and simplify some CSS but it’s too distinct for this PR.
I’ll take another look anyway. I don’t love the classes either.
stokesman
force-pushed
the
update/editor-header-layout
branch
from
6406918
to
884c534
Compare
|
Let’s 🚢. Thanks everyone who tested and reviewed. |
What?
An improvement to center the Document Bar and prevent layout shifts at > medium breakpoints when switching between page and template.
Why?
How?
Uses grid instead of flex because that was the only way I knew to resiliently center the Document Bar. There is a bit more to it than that but I think it may be better to cover that with some review comments.
More
The layout shift used to not happen but now in template editing the "View Page" button hides and causes it. Maybe the button shouldn’t hide and instead merely disable #53913 (comment) but it still seems like a good idea to make the layout more stable (and truly centered).
Testing Instructions
Screenshots or screencast
Layout on trunk editing a page
Layout on trunk editing a template
Layout on this branch editing a page
Capture of editing a template on the branch omitted because there’s no layout shift.
Trunk switching between page and template
shifty-document-bar.mp4
This branch switching between page and template
stable-document-bar.mp4