FSE Navigation Sidebar: Move navigation sidebar in DOM hierarchy #25884
Conversation
jeyip
added
[Status] In Progress
|
Size Change: +314 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
jeyip
force-pushed
the
update/navigation-ui-placement
branch
from
5b00104
to
d2f8648
Compare
jeyip
force-pushed
the
update/navigation-ui-placement
branch
3 times, most recently
from
85572e6
to
390e69f
Compare
| ) } | ||
| <div className="interface-interface-skeleton__body"> | ||
| { !! leftSidebar && ( | ||
| <div className="interface-interface-skeleton__layout"> |
There was a problem hiding this comment.
Note:
Previously, the navigation panel and navigation toggle lived within skeleton__body and skeleton__header, respectively. In order to address unexpected keyboard focus order between the nav toggle and nav panel, we create a new element inside of interface-skeleton called skeleton__navigation-sidebar.
To account for the layout changes of introducing a new element, we add two wrapper classes: skeleton__site and skeleton__content. Both ensure that the skeleton__header and skeleton__body shrink their children correctly when the navigation sidebar is open.
| @@ -6,6 +6,8 @@ | |||
| background: $gray-900; | |||
| border-radius: 0; | |||
| display: flex; | |||
| position: absolute; | |||
There was a problem hiding this comment.
Note:
When the navigation sidebar is closed, the skeleton body (i.e. the block editor) no longer spans the entire width of the viewport. The sidebar (and toggle) occupy space at the left edge of the screen. To address this, we position the navigation toggle absolutely when the sidebar is closed.
packages/interface/src/components/interface-skeleton/style.scss
Outdated
Show resolved
Hide resolved
jeyip
requested review from
david-szabo97,
Copons,
vindl,
noahtallen and
Addison-Stavlo
| onClick={ onNavigationToggle } | ||
| /> | ||
| { content === 'navigation' && isOpen && <NavigationPanel /> } | ||
| </> |
There was a problem hiding this comment.
Should we keep the MainDashboardButton.Slot removed from packages/edit-site/src/components/header/index.js?
There was a problem hiding this comment.
I think it should now wrap Back to Dashboard button, instead of the main one that toggles the sidebar. That's what it does in post editor too.
|
The behaviour here feels good to me, and the keyboard navigation is exactly as I would expect. As mentioned elsewhere, I'd even go as far as removing the auto-focus on the first button inside the navigation (Back to Dashboard) on sidebar toggle, since now it's only as far as a simple tab press. Most obvious change required: replace all the new z-index values with the About the I don't see any obvious regressions, but I'm wondering if there's a way to tidy it up a little bit.
|
Great catch! You were spot on about why I initially implemented
Agreed. I'll give this more thought and circle back tomorrow. For now, I'll use drawer instead of navigation-sidebar. |
| isNavigationOpen | ||
| } | ||
| onNavigationToggle={ () => | ||
| toggleLeftSidebarContent( |
There was a problem hiding this comment.
(More of a pondering than a request)
Given that now the navigation sidebar is completely separated from the left sidebar, I think they should be handled by different states (e.g. setLeftSidebar and setNavigationSidebar).
With different states we could even have both sidebar visible at the same time.
I don't know if it's something we want, though, as we might end up with 3 sidebars open at the same time, which feels very cluttered to me.
Either way, we can make navigation and left sidebars "interdependent": when you open one, automatically close the other.
There was a problem hiding this comment.
I wanted to put the nav state into redux so I could use it in a random component, so i tried this out here: #26003
| @@ -1,7 +1,6 @@ | |||
| .edit-site-navigation-panel { | |||
| animation: edit-site-navigation-panel__slide-in 0.1s linear; | |||
| height: 100%; | |||
There was a problem hiding this comment.
(Not caused by this PR, but it's more a mental note to address it at some point)
The panel is 100% height, but in fact it should be slightly shorter: it's pushed down by the header (60px + 1px of border), and it's supposed to end above the footer (25px, only on "> mobile").
I guess we could remove those 86px from the height with calc() or as a bottom padding.
Somewhat related: the Navigation component hides both overflows, while it only really needs to hide overflow-x.
Hiding overflow-y means that if the menu is taller than the screen, it gets cut off.
…sidebar Navigation sidebar and inserter sidebar state were both moved into the 'core/edit-site' data store. We incorporate any relevant actions and selectors into our source code updates.
Copons
force-pushed
the
update/navigation-ui-placement
branch
from
e68d1ab
to
4a2937a
Compare
No worries! Rebased the PR and dropped the rogue commits. |
|
@Copons Thanks for merging this! 🙏 |
| @@ -56,6 +56,7 @@ $mobile-header-toolbar-height: 44px; | |||
| $mobile-floating-toolbar-height: 44px; | |||
| $mobile-floating-toolbar-margin: 8px; | |||
| $mobile-color-swatch: 48px; | |||
| $header-toolbar-min-width: 335px; | |||
There was a problem hiding this comment.
@jeyip I know this value is iPhone friendly, but it doesn't appear to work as intended as the mobile experience is otherwise broken. The variable is also used only in this PR and no-where else. I'd recommend not adding this variable, and instead either use $break-mobile from _breakpoints.scss so as to avoid fragmenting the responsive dev.
We also have some good mixins that help mobile dev, such as @include break-small() { ... } to abstact and keep things in sync.
There was a problem hiding this comment.
The 335px was meant to ensure a certain width for the toolbar on either side of the header used here and here. The min width prevented the left and right toolbars from collapsing as the browser viewport was shrunk horizontally. Giving this more thought, it should absolutely be a desktop specific affordance. I'm happy to open another PR incorporating the break point mixins.
I'd recommend not adding this variable, and instead either use $break-mobile from _breakpoints.scss so as to avoid fragmenting the responsive dev.
I'm not quite sure I'm tracking here. I think the name of the variable may be misleading, because it sounds like we see the 335px as an iphone friendly breakpoint?
There was a problem hiding this comment.
Ah! Thank you for the explanation. I initially pondered where those 335px came from, because it seems a relatively arbitrary number given the base8 grid most of the UI uses. I then thought — maybe it's that somewhat odd iphone resolution which I remembered as 335px (iPhone 6: 750 x 1334, which makes it 375 points horizontally) but as you can see, I was wrong.
With your explanation, though, I think a different approach is needed for min-width. Longer term I think we need to stack the toolbar if there isn't space, but nearer term I think smaller steps can be taken.
I'm happy to open another PR incorporating the break point mixins.
#26021 does that, and but also hides some buttons, and minimizes labels, as the viewport approaches mobile breakpoints. It's only a first step, and more are needed, but it's a good step. It's currently blocked on the white screen as shown in the intro of the PR, which happens because the left sidebar is always output, regardless of open or not. I could use help with that if you have bandwidth!
I'm not quite sure I'm tracking here. I think the name of the variable may be misleading, because it sounds like we see the 335px as an iphone friendly breakpoint?
To be specific, we should only be creating new variables in the _variables.scss file if they are meant to be used widely and in multiple places. If it's just for the site editor as is currently the case, I'd just type the content of the variable directly where needed, or just register the variable directly in those files.
I'd look at finding a different approach than the min-width, though (not urgently) it seems a half-step towards solving a problem that needs a more foundational mobile improvement. The work could probably coincide with enforcing of the stacking top toolbar, so we fix this:
I know it's beta! I know there's a lot of work to be done. I'm just:
- excited about the site editor
- of the belief that if we test mobile in every PR, even if it's not superb, we minimize future headaches
|
This PR appears to consider mobile (specificaly the iPhone based on the 335px variable introduced), but the mobile experience remains broken, so I'm curious how this was tested for mobile. For me, the site editor is just a white screen on mobile, something which #26021 fixes, but even after that fix, the 335px variable appears to not really address any mobile issues. I really appreciate all the work you're doing on this, and I know it's super early to optimize for a breakpoint that is likely going to be rarely used for building templates. However this will nevertheless need to be considered at some point, and the sooner we start testing it, the less technical debt we will have to revisit and fix. Most of the time it's it's enough to wrap desktop-specific affordances in a |
Thanks for your thoughts @jasmussen. They're much appreciated. To clarify, although the PR appears to consider mobile (like the 335px variable), I didn't explicitly take mobile screens into account, which admittedly might be an oversight. I only ensured that the desktop behaved responsively up to the point where the tablet / mobile break-points take over. I've seen the mobile white screen for a while in site editor development, which is part of the reason why I didn't test behavior at smaller screen widths. I should've, however, been more proactive about calling out the unexpected behavior that you're addressing in #26021.
I think these are very valid points, especially if we plan on supporting mobile site editing in the future. It's tough developing for a mobile experience without an idea of what it will look like, but I agree that we can circumvent a lot of issues with desktop-specific affordances wrapped in breakpoint mixins. |
Absolutely. And in absence of full knowledge of that, I'd just test site editor PRs against a viewport that's <600px wide and see how broken it is. As we move forward, we can then raise the bar for what brokenness we're willing to accept. |
priethor
added
the
[Focus] Accessibility (a11y)
Description
The focus order for the navigation panel wasn't behaving as expected. Shift+Tab from the back button did not focus the "Toggle Navigation" button, but rather the last element of the header toolbar instead.
Although consistent with the Inserter's behavior, Shaun has identified inserter behavior as a bug (source). The goal is to update the placement of the navbar toggle and panel so that focus navigation behaves as expected.
How has this been tested?
Site Editor
Post Editor
Screenshots
Before
After
Types of changes
Bug fix #25701
Checklist: