-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update: Use offsite navigation editor on the navigation inspector sidebar. #46440
Update: Use offsite navigation editor on the navigation inspector sidebar. #46440
Conversation
Size Change: +41 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
a373c93
to
a1900ac
Compare
This looks like a good start. I noticed quite a few things don't work:
|
Hi @scruffian, the inserter edit, etc, are hidden without the experiment enabled. I guess we can merge as these features are hidden without the experiment and then look into fixing these issues in the OffCanvasEditor component? |
That's true, but if we're going down this route we should probably put the behind the same experimental flag, so it only shows up when people opt in. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With respect, I really think we should have everything related to the offcanvas editor behind the experiment flag.
Why?
The engineering decisions behind that offcanvas have been taken to prioritise expediency to allow for early testing and UX feedback. As a result it is not suitable for exposure outside of the experiment.
Doing that at this time could hinder progress on the offcanvas experiment as bugs would need to be fixed and trunk
more closely monitored for breakages. At this time we're still waiting on feedback from the community as to whether offcanvas is a good experience.
If that makes the Navigation
element of the Navigation panel redundant then it should be removed for now or again be placed behind the experiment flag.
a1900ac
to
a1534b9
Compare
a1534b9
to
81f1947
Compare
Hi @getdave, thank you for sharing your thoughts. I updated this PR to use the new component if the flag is enabled. On the browse sidebar, we will also only show the navigation menu item if the experiment is enabled. I think this addresses your concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@jorgefilipecosta Thank you for being so considerate and apologies for blocking with that review 🙇 |
This PR updates the navigation inspector sidebar to use the new navigation offsite editor being worked on:
https://github.com/orgs/WordPress/projects/60/views/1
If the navigation offsite editor is disabled, the edit button and the appended are not shown, so it behaves similarly to what we have now.
This change also allows browse mode navigation menus to take advantage of the new menu editor. After this PR, and #46436 are merged.
cc: @scruffian
Screenshots or screencast
Off-canvas editor enable:
Off-canvas editor disable: