Document Actions: Fix document title misalignment with an open nav sidebar #25630
Conversation
jeyip
added
[Type] Bug
jeyip
requested review from
Addison-Stavlo,
Copons,
david-szabo97 and
noahtallen
|
Size Change: +74 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
Addison-Stavlo
left a comment
Addison-Stavlo
left a comment
There was a problem hiding this comment.
This fixes it, but I'm a little confused regarding the extra wrappers, classnames, and added style rules in general. Simply moving the MainDashboardButton.Slot out of the edit-site-header_start wrapper solves the issue on my end.
|
i thought that approach might fix it too, but since the side button is no longer part of
(I think that could be fixed by adding absolute and some padding) |
Ahhh yeah! I see that now. |
I updated the classes because they were previously based on positioning (start, center, end). With this fix, I don't feel strongly about the changes though, so if we feel like they're unintuitive, I'm happy to revert them. @Addison-Stavlo |
Actually regarding the name changes themselves they make sense to me! I was just confused why there a handful of things changing when moving that sidebar out of the wrapper seemed to solve the issue (which it really didn't as Noah pointed out). |
|
I added a few notes to the PR subheading "Types of Changes" to elaborate more on what the additional styling is meant to do |
A nitpicky point on this is that we're really trying to align with editor content all the time (I think). It just so happens that it matches the viewport when it's fullscreen :) |
If we have a scrollbar in the content then the document title is a little bit off. Since the editor's content container's width is smaller with a few pixels because of the scrollbar. Without scrollbar With scrollbar |
There was a problem hiding this comment.
One thing I am noticing is the difference in overlapping behavior on smaller screen widths / devices.
With these changes, when we reduce the screen width the items on the left overlap our name panel:
On the current master branch, this is prevented given how the style rules are set:
It would be nice to preserve this behavior, however it sounds like the double-dropdown that it is conflicting with will be removed in the future and I don't think there is an issue with the remainder of the items on the left (unless more are added in the future) as the top toolbar will disappear before we get to that narrow of a screen width. Given that I am not sure how concerning this should be. 🤔
@david-szabo97 @Addison-Stavlo Great calllouts. I'll see if I can't address them or think of an alternative approach altogether to center the document title. |
| $button-size-small: 24px; | ||
| $header-height: 60px; | ||
| $panel-header-height: $grid-unit-60; | ||
| $nav-sidebar-width: 300px; |
There was a problem hiding this comment.
Note:
I'd prefer to scope this variable to the site editor package (in a new _variables.scss in src/edit_site), but it looks like the prevailing convention is to add variables to this global base-styles stylesheet. If anyone knows otherwise, I'm happy to make changes.
|
Thinking more about this:
I don't think we should be too concerned with this side of things. Consider that many users may often have the 'block-inspector' toolbar on the right hand side of the editor open a majority of the time, this shifts the editor canvas to the left by a the width of that sidebar. We probably shouldn't be worried about it being perfectly centered over the editor canvas in all cases, but centered with respect to the topbar area/items itself? |
That's a good observation. I never noticed that opening the block settings sidebar changes content width.
Given this information, addressing the scrollbar width seems less important. I do, however, think that we should fix the problem eventually. I imagine that there is still a large contingent of users that do not have block settings open when editing content. If they have a scrollbar permanently displayed, the document title will always be slightly misaligned. How would we feel about creating a separate issue that's lower priority and handling the scrollbar in the future? @david-szabo97 @Addison-Stavlo |
noahtallen
left a comment
noahtallen
left a comment
There was a problem hiding this comment.
yeah, I think the margin causes the center div to be pushed over further than it should be when the distance between "left" and "right" is small enough. So this makes things messy at even medium-sized viewports.
I also noticed this issue:
|
@noahtallen @Addison-Stavlo I updated the styling. The title should be centering correctly and all problems should be addressed, except the misalignment when a permanent scrollbar is present that @david-szabo97 called out. I elaborate on that here. |
Yep! This is kinda out of scope for this PR. Let's do it separately. |
noahtallen
left a comment
noahtallen
left a comment
There was a problem hiding this comment.
other than the one thing about adding the classname to the body tag, I think this is about ready to go!
I'm noticing it still has two issues, but they already exist, so they don't have to block this.
The first is that the title looks off-center when the right sidebars are open. I know it's still centered in the expected spot, but it just looks visually weird. I'm unsure what our expectations should be for this, so I think we can leave it be for now.
This is probably a bug with the nav sidebar rather than this, but at smallish viewports, the size of the sidebar in the header is different from the size of the sidebar in the body
jeyip
force-pushed
the
fix/document-title-misalignment-with-open-sidebar
branch
from
cb386d9 to
f3b9863
Compare
jeyip
force-pushed
the
fix/document-title-misalignment-with-open-sidebar
branch
from
f3b9863 to
a2d40e6
Compare
The same thing happens when the inserter sidebar is opened 🙃 |
|
Created an issue for the scrollbar misalignment @david-szabo97 #25716 |
|
nice, the new body class is looking good! Left a nitpicky comment about code location, but this is ready to merge whenever you like ;) |
Description
The document title in the site editor isn't center aligned with content when the new navigation sidebar is open.
How has this been tested?
Screenshots
Before
After
Types of changes
Bug fix which addresses #25629.
Elaborating more on the approach, there are two scenarios we have to support.
Moving the nav sidebar component outside of the
edit-site-header_startwrapper with the existing flex layout solves # 2. To handle situation # 1, I absolutely position the document title in the middle of the page. Opening the sidebar just removes all references to absolute positioning. Closing the sidebar reintroduces absolute positioning.Checklist: