Try: Force stacked top toolbar. #30234
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
the
[Feature] Site Editor
|
Size Change: -518 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
|
Yes please! This makes more sense for tabbing too. It would also enable us to unify all the toolbars (mobile, top, popover) to be the some component with slight differences in position. |
|
Yay!
✅
✅ Working well so far. It looks a little strange when the toolbar is empty:
But without some radical changes I don't have any good ideas on how we might remedy this. |
I actually think there's room for some radical changes! I recall some Pablo mockups around the fixed toolbar. I think it's time those could be revisited. I'll leave this for some more feedback, I'd like to get all the technical stuff in a good place, it's really deep surgery. But once that is solid, either as part of this PR or in a followup, I'd like to look at some of that. Edit: Even something simple, like stacking the toolbar so it lines up with the page/block tabbar might be a good start. |
jasmussen
force-pushed
the
try/force-stacked-toolbar
branch
from
7a9bc33 to
2be7654
Compare
|
I've hit a bit of a wall with this one. There's a lot of good CSS and code cleanup here, it would be a shame that went to waste. But as I've continued to tinker, it's become clear that instead of this:
... we need this:
Note how the sidebar starts right below the topmost toolbar, not below the stacked one. I tried moving the block toolbar around again in the editor skeleton, but failed to do so. If anyone has time to help me with that, I'd appreciate it — feel free to push directly to the branch. |
youknowriad
force-pushed
the
try/force-stacked-toolbar
branch
2 times, most recently
from
63ed77f to
89aff2d
Compare
|
@jasmussen made some tweaks to try to achieve what you suggest. Aside, I noticed some horizontal scrollbars on the inspector (not sure it's related to my changes cause I don't think I did something that impacts that but you never know) |
|
Thanks so much, I saw you tinkering! I need to step away for a bit and have some late, late lunch, but I'll try and polish when I'm able. I did notice, however, that we still have the same toolbar placement as shown in #30234 (comment), where I was hoping we could move it down one step further and get this:
What do you think? Edit: Note how the top toolbar in that view is flush with the tabbar for the sidebar. |
youknowriad
force-pushed
the
try/force-stacked-toolbar
branch
from
89aff2d to
56fe438
Compare
|
@jasmussen you probably didn't update your branch properly, I see the same thing as your screenshot. |
|
There's some jumpiness now when you select a block and the toolbar appears, I think we can solve that though by using a "height: 0" on the toolbar wrapper + margin/padding to the canvas. |
|
Another possibility would be to actually show something in the toolbar if no block is selected. |
|
For the site editor, I suspect a white empty toolbar is preferable to showing excessive top padding. But I'll take a look when I get a moment. |
|
I swear I've checked out fresh, I deleted this local branch entirely, pulled, then checked out. But I still see this:
What I mean is I'd like to see this:
|
|
well, someone is doing something wrong :P It could be me or you but we need to figure it out :) |
|
Oh wait, you're looking at the site editor, I'm only working on the post editor. |
|
Why is the site editor's skeleton different? 🤔 |
7 tasks
|
Closing this one in favor of #31134. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When the "Top Toolbar" option is enabled, the block toolbar is integrated into the main toolbar but only on wide breakpoints. This very often breaks the responsive layout, as the toolbars are arbitrary in width:
On smaller viewports, the block toolbar is stacked, which makes the tab-order mismatch the visual order:
Because of how the stacked toolbar was implemented, it was an absolutely positioned overlay which covered content. This was mitigated in the post editor, because the "Title" block most commonly made room for the top toolbar to cover content, but in the site editor, this is problematic:
This PR fixes that by forcing the top toolbar to be stacked always:
This has a number of benefits:
Note, this PR is still a work in progress:
But for now, I'd like your thoughts and reviews:
How has this been tested?
Please test the post and site editor on mobile, without the top toolbar option enabled — it should still be stacked.
Please test the post and site editor on any viewport width, with the top toolbar option enabled.
Verify that the top toolbar always stays stacked, tabs well, works as intended, and neer covers content.
Checklist: