Navigation: Try removing "Add all pages" from placeholder. #36775
Conversation
jasmussen
added
[Type] Enhancement
|
Size Change: -46 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Recent anecdotal feedback from a friend, "Why did it create all these pages that I don’t need?", suggests that the "Add all pages" option isn't necessarily the most obvious one.
It makes sense to me. In testing I find myself using it all the time, but in real life Navigation Menus are, generally, just a really specific, important set of pages. Taking as an example GitHub itself, "Pull requests, Issues, Marketplace, and Explore" aren't all the pages they have, and the set of pages displayed in the footer proves that point.
e2e test failure seems unrelated, it's not failing locally.
|
Thanks for the review! I agree with all of this, and it's the motivation for the PR. I'll let it sit for Isabel to have a chance to chime in, and then hopefully land it some time tomorrow. |
There was a problem hiding this comment.
For what it's worth I think this would be a good simplification of the block.
It is a small code change I found no problems in my manual test.
One question -If there are no menus already saved, is the "start empty" still relevant?
It is then the only option you have, and you have to manually select it, it can't be skipped.
|
Thanks so much for the reviews.
Excellent question, and I think it's one of the pieces still being worked out, and why despite both your much appreciated blessings, I'll await a sanity check from Isabel. I think she has insights on this that I don't have. |
|
I agree with the change in principle 😄 , but it would be good to look into this scenario:
There is also some logic around handling "add all pages" vs "start empty" that should be removed now there's only one of these options. But most importantly, before we go ahead with this PR, I'd like to see #36760 merged, because that one is a blocker for 5.9 and it touches the same code (it removes the menu naming dialog). Let's get that out of the way first, and then I'm happy to help with this experiment, and any conflicts that result. |
|
Thank you for the sanity check! I'll mark this one as blocked for now and we can revisit.
Imagining beyond near term solutions and just speculating on #36667 (which notes a lack of a dropdown for selecting menus!), that interface seems to suggest that there could be a default menu — just all the pages on your site — shown in that interface on a fresh install where a user never explicitly created a menu in the first place. If that came into being, I imagine a navigation menu could skip the placeholder step entirely, and just show that? |
jasmussen
added
the
[Status] Blocked
It should be possible to skip the placeholder if there are no existing menus, but if menus have already been created, it makes sense to show the placeholder with the dropdown, so users have the choice to re-use an existing menu. |
tellthemachines
force-pushed
the
try/navigation-remove-all-pages
branch
from
898c755
to
30541f8
Compare
|
I fixed the conflicts and removed the dead code, but perhaps we should hold off on this until Beta 1 (unless we want it to go into 5.9?) to avoid potential conflicts with any other urgent nav work that's being prepared. I'd also like to look into not showing the placeholder if there aren't any menus, once the 5.9 rush is over 😅 |
|
Thank you as always!
I don't have a very strong opinion and I'd be happy to defer to you or anyone with a strong opinion. I consider this particular PR to be 1 of 3 (with the other two being #36778 and #36858) that are important to improving the initial setup state. I like the idea of those three being ready to go at a moments notice, should they suddenly become a priority. That brings me to the genesis of the changes, which is recurring feedback from @annezazu's outreaches, notably around the gray loading-state-like blobs.
Oh from my perspective that's mostly a a notion for an optimization, and not necessarily a clear task. Thanks again! |
jasmussen
force-pushed
the
try/navigation-remove-all-pages
branch
from
f15f36c
to
e5fa450
Compare
jasmussen
removed
the
[Status] Blocked
|
I've rebased this one, would love to land it. |
jasmussen
force-pushed
the
try/navigation-remove-all-pages
branch
from
6966c38
to
778e58e
Compare
There was a problem hiding this comment.
Thanks for wrangling this @jasmussen 👏
I tested this and it does successfully remove the All Pages option ✅
Whilst I support the proposal in principle, I do wonder whether we're acting too soon on changing the block UI before we have time to digest the (user) feedback we receive from WP 5.9.
That said, this seems to have broad support from contributors so I wouldn't want to hold this up if folks really believe it's a good choice.
As noted by @carolinan, I would like to see us evolve the UI such that if there are no options other than Start empty in the placeholder, then we simply bypass the placeholder entirely. Do we have an Issue tracking this as a requirement?
🎉 I wholly love that idea. No, I don't believe an issue exists yet dedicated specifically to this. Some of the things to discuss, I imagine, are:
For when zero menus are created, we need to think about #36667. In my reading, what it says is that if no menus have been created yet, assume the page structure of a site is the menu, and load that by default. It isn't until you customize the sorting order, or delete or remove an item from the menu, that an actual menu is saved as a CPT. What do you think? |
jasmussen
force-pushed
the
try/navigation-remove-all-pages
branch
from
778e58e
to
0eeb369
Compare
|
I'm unsure why the static analysis check is failing 🤔 |
I suspect it's unrelated but you could try running it locally to see if you get more error output. |
|
It's almost certainly my own fault #38235 🙈 Apologies, folks! Fix should land soon. |
jasmussen
force-pushed
the
try/navigation-remove-all-pages
branch
from
0eeb369
to
7e13dac
Compare
|
I'll let this one sit overnight for any new thoughts before I land it. |
|
Alright, I'll land this one but be happy to follow up on anything. |
Description
As the navigation block is evolving with persistance, the placeholder state may play a smaller role in favor of pre-built menus and patterns that leverage them. These patterns might include the Page List by default, or the menu does. What remains are two ways to start a new menu: empty or with the "All Pages" block preinserted:
Recent anecdotal feedback from a friend, "Why did it create all these pages that I don’t need?", suggests that the "Add all pages" option isn't necessarily the most obvious one. And in light of the changes to persistence, it might indeed make sense to just not include the option at all: it can still be added manually, or as part of a pattern. So this PR removes it:
What do you think?
How has this been tested?
Add a navigation block, see that it includes only two options.
Checklist:
*.native.jsfiles for terms that need renaming or removal).