Update: Hide page list on single page list menus. #48725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
There's a small moment where the "page list" block is available but its children didn't load yet where you see "page list" and then a second later it gets removed. I think that's a bit awkward, maybe we should remove the condition that checks inner blocks length. |
|
The other option which is probably better is to actually find a way to keep the "isLoading" flag true while the page list is loading its inner blocks. |
|
Size Change: +206 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/hide-page-list-on-single-page-list-menus
branch
2 times, most recently
from
c710ddc to
5fb49dd
Compare
This would be ideal. But I'm not sure how we can do that.
I followed this suggestion 👍 |
|
The problem right now is that I can see this for a couple seconds before the items actually show up:
|
I will check the page-list block and see what query it does. Maybe we can have a loading state while that query is requesting. |
jorgefilipecosta
force-pushed
the
update/hide-page-list-on-single-page-list-menus
branch
from
5fb49dd to
6e2518f
Compare
|
The issue was fixed now we wait for the pages to load. |
|
This seems to work quite well. Since re-ordering isn't possible, we should change the cursor on hover from drag to pointer. The focus state is a bit strange when the ellipsis/lock icons are hidden:
It might be something to fix in #48675, but it's possible to select the Page List block, which feels strange as an isolated interaction, but it also applies a background to the child blocks which looks broken:
After adding a block, the Page List structure appears which is good, but it appears collapsed. Would it be possible for it to appear in the expanded state? That might alleviate any "where did my pages go?" concerns. |
|
Flaky tests detected in dfb9d5c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4574396331
|
youknowriad
reviewed
Mar 3, 2023
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { unlock } from '../../private-apis'; | ||
| import { NavigationMenuLoader } from './loader'; | ||
|
|
||
| // Needs to be kept in sync with the query used at packages/block-library/src/page-list/edit.js. |
There was a problem hiding this comment.
Should we add a similar comment on the other file too, to avoid people missing this if they change the other file. Also, maybe add a sentence here to explain why we use it. Some thing like
"We use the query to only render the menus once everything is loaded." (or something like that)
youknowriad
reviewed
Mar 3, 2023
| @@ -61,3 +61,7 @@ | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| .edit-site-sidebar-navigation-screen-navigation-menus-off-canvas-container.is-single-page-list .block-editor-list-view-block-select-button__lock { | |||
| display: none; | |||
There was a problem hiding this comment.
Do we want to remove the locks only when it's "single page list". In other words, I'm wondering whether is-single-page-list classname is necessary.
There was a problem hiding this comment.
If it should be a global change, maybe we can also avoid the extra div. As you can see, a few lines above in this style file, we override other off canvas menu styles as well.
There was a problem hiding this comment.
Meanwhile, it was applied globally, and this rule was removed.
navigation-loading.movAs you can see on the above screen share. There's a "glitch" when the navigation is being loaded. You can see the loading showing, then disappearing for a fraction of a second and then appearing again. Is there a way we can keep the loading state visible until the end. There might be some gaps in the condition: a time between the inner blocks becoming controlled and the page list starting to load. Not sure exactly how to cover that though. |
|
Just connecting a dot, there are a couple other minor issues with the loading state that I outlined in #48682 |
Mamaduka
added
[Feature] Site Editor
jorgefilipecosta
force-pushed
the
update/hide-page-list-on-single-page-list-menus
branch
2 times, most recently
from
baad085 to
d58d261
Compare
Yeah unfortunately between blocks becoming controlled and pages starting to load there is an issue. The only fix I found was to add a small delay before stopping the load. It seems to be working well now. |
jorgefilipecosta
force-pushed
the
update/hide-page-list-on-single-page-list-menus
branch
from
d58d261 to
a1f2679
Compare
Squashed commits: [c6f61bf] Update: Hide page list on single page list menus.
|
Given that all the feedback is added and all the tests are passing, I'm going to merge this PR. |
jorgefilipecosta
deleted the
update/hide-page-list-on-single-page-list-menus
branch
This was referenced Jul 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #48714
This PR hides the page list block in the navigation sidebar in cases where we have a single page list block with at least one-page item.
It is still possible to use the inserter to add a sibling block after the page list and in that case, the menu becomes complex with the page list visible.
Screenshot