Site Editor: Add navigation panel with placeholder content #25506
Conversation
david-szabo97
added
[Feature] Full Site Editing
[Feature] Navigation Component
david-szabo97
requested review from
vindl,
Copons,
jeyip,
noahtallen and
Addison-Stavlo
|
Size Change: +873 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
| @@ -89,7 +91,10 @@ export default function Header( { | |||
| <div className="edit-site-header"> | |||
| <div className="edit-site-header_start"> | |||
| <MainDashboardButton.Slot> | |||
| <FullscreenModeClose /> | |||
There was a problem hiding this comment.
This button is exported from the package (experimental for now) and provides a way for 3rd plugins to customize some of its parts while retaining functionality (see docs here). Given that it will no longer represent a close action, I think that we should rename the existing FullscreenModeClose to NavigationButton or DashboardButton and place our code changes there.
There was a problem hiding this comment.
We should also figure out how to handle the FSE Navigation when not in fullscreen mode, where we wouldn't have the WP/Site Icon back button anymore.
There was a problem hiding this comment.
Good point @Copons! I don't consider it a blocker for this PR though, as it's something that we can figure out later.
david-szabo97
force-pushed
the
try/navigation-component-in-site-editor
branch
from
a766d9e to
faeff3d
Compare
|
#25507 is merged so I did a rebase. Updated the description with the new screenshots. |
|
@shaunandrews I have a question about the intended behavior here. Opening the inserter will close the navigation sidebar and vice versa in present version. Is that fine or should it be possible for both sidebars to be open at the same time? Also, how should it interact with potential Persistent block navigator panel? |
|
david-szabo97
changed the title
There was a problem hiding this comment.
Thanks @david-szabo97! This looks like a good starting point for our upcoming iteratons.
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,42 @@ | |||
| .edit-site-dashboard-button_wrapper { | |||
There was a problem hiding this comment.
Please note that we should use a "loose BEM" naming.
Modifier should be preceded by a double underscore.
E.g. .edit-site-dashboard-button__wrapper
(There are a few other occurrences of this in the PR, but I'll leave you the fun of finding them 😛 )
There was a problem hiding this comment.
I was confused by FullscreenModeClose because that component is not following the BEM. 🤔 Thanks for letting me know, fixed it up!
| <DashboardButton | ||
| isOpen={ isNavigationOpen } | ||
| onClick={ onToggleNavigation } | ||
| /> |
There was a problem hiding this comment.
I've got a few doubts about this change.
While it's true that this button doesn't close fullscreen mode anymore, it also doesn't navigate back to the dashboard (so the "Open dashboard" label in the component is misleading) — and it's not even technically just a button 😄.
What it does now is to toggle the navigation sidebar, so I'd be more inclined in calling it NavigationPanelToggle.
I think we could also keep FullscreenModeClose.
While it would be left unused in this PR, we could update it to become (be?) the actual "dashboard button".
Consumers relying on it would not break, and we could use it inside the navigation toggle, or in the navigation itself (or wherever we see fit).
Either way, my opinion is that we should probably avoid removing existing components from an experimental PR whose purpose is to be a starting point, and not the finished product.
There was a problem hiding this comment.
I agree that that name is more appropriate. Also no objections to keeping the FullscreenModeClose if we are going to repurpose it and include it in navigation.
There was a problem hiding this comment.
My point is also that the navigation sidebar is still in flux, and I'm not super keen on removing an established component (albeit experimental).
We might end up not using it, or heavily modifying it, but let's care about that when we'll be actually working on the back navigation. 🙂
There was a problem hiding this comment.
- Renamed
DashboardButtontoNavigationToggle - Reverted
FullscreenModeClosecomponent, let's keep it for now and we will iterate upon it
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
david-szabo97
force-pushed
the
try/navigation-component-in-site-editor
branch
from
0d2d6b6 to
76d897b
Compare
There was a problem hiding this comment.
This LGTM!
Thank you @david-szabo97 for working on this! ✨
I'll open an issue for future discussion of the FullscreenModeClose destiny.
| import InserterPanel from './inserter-panel'; | ||
| import NavigationPanel from './navigation-panel'; | ||
|
|
||
| const LeftSidebar = ( { content, setContent } ) => { |
There was a problem hiding this comment.
Not a blocker, but we should rename LeftSidebar (and any other left prefix) to something more semantic. This will be a Right sidebar on RTL sites and more of a drawer (or something) on small screens. Material design uses "side sheet" for a similar concept. We could also just simply call it Sidebar (or NavSidebar) and drop "left" altogether.
|
First observation: the
In the Site Editor, it looks like this 🙂 :
where apparently there's some missing CSS. Besides that, I'm not sure using the same UI control for different purposes on different screens is a good user experience. As a user, If I know the W button in the block editor brings me back to the Posts lists, I would expect the same button in the Site editor to do the same. |
|
Worth also noting the "sliding in sidebar" pattern is undergoing discussion by the accessibility team as it's a pattern that introduces various accessibility issues. It does relate to other new sidebars as well though so it's not specific to this PR. An audit is going to be made on the issue related to the main Inserter, that was already identified by the accessibility team as an accessibility regression in WordPress 5.5 compared to the implementation in 5.4. See #24975 |
|
@afercia Thanks for chiming in! The plan here was to merge the sidebar in the Site Editor in order to work on it in a real scenario, but still "protected" by the fact that it's an experimental environment. We have been rushing this PR in because we (as in: people working on the Site Editor) are trying to improve the whole UX, which is arguably not very good right now. Thank you for linking that accessibility issue. |
Related: #25260 #25261
Description
Putting Navigation component in action! Great way to see how it cooperates with the site editor. Also acts an example of how we could implement the preview popup.
This is a minimal implementation to see the Navigation component in the Site Editor.
Needs fix
How has this been tested?
yarn wp-env startScreenshots
Closed:
Open:
Preview:
Checklist: