List draft navigations in Browse mode Navigation section #51422
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
|
Size Change: +135 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5067d103867088278b2bf2b0b179c1f981961c91. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5323755413
|
getdave
changed the title
getdave
added
the
[Feature] Navigation in Site View
|
I updated this to add a |
getdave
reviewed
Jun 13, 2023
Comment on lines
-6
to
+8
| status: 'publish', | ||
| status: [ 'publish', 'draft' ], |
There was a problem hiding this comment.
We need to amend the preloading in https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.3/navigation-block-preloading.php
Also can we add a link to that file as a code comment so folks are aware of this in future?
There was a problem hiding this comment.
I think this change might mean we can actually only have a single preloaded request which would be excellent. Needs to be carefully tested though.
getdave
reviewed
Jun 13, 2023
| @@ -19,6 +19,25 @@ import SidebarNavigationItem from '../sidebar-navigation-item'; | |||
| import { PRELOADED_NAVIGATION_MENUS_QUERY } from './constants'; | |||
| import { useLink } from '../routes/link'; | |||
|
|
|||
| // Copied from packages/block-library/src/navigation/edit/navigation-menu-selector.js. | |||
There was a problem hiding this comment.
Note: I'm happy with this duplication. However let's track it in #51455
|
I have updated the two preloaded endpoints so that they are almost identical. However one of them uses the _locale param and the other does not. It looks non-trivial to get these aligned - one endpoint it called using |
getdave
reviewed
Jun 13, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/constants.js
Show resolved
Hide resolved
I agree this PR shouldn't be concerned with normalizing any preloaded queries. As long as the changes to the query in the JS has been updated in the PHP then it's fine. |
|
Are we sure we want the drafts to show up in the browse mode sidebar? I feel as if drafts are a lower level type of data whcih should be found in a more expanded view not in the sidebar. |
A good question. We do already show these in the navigation block, so I think if we don't want them here we shouldn't show them there either. When you create a new navigation it's in a draft state, so we probably need to show it... |
|
I think we should show it but we need to be clear about the statuses of each menu. |
draganescu
approved these changes
Jun 16, 2023
getdave
reviewed
Jun 19, 2023
| @@ -71,7 +71,12 @@ function selectNavigationMenus( select ) { | |||
| const args = [ | |||
| 'postType', | |||
| 'wp_navigation', | |||
| { per_page: -1, status: [ 'publish', 'draft' ] }, | |||
| { | |||
There was a problem hiding this comment.
You'll need to amend the tests with this query.
I think perhaps these tests are too tied to implementation details 🤔
|
@getdave I consolidated the settings into a constant, so that it's kept in sync in both places it's used. Does that look good to you? |
getdave
reviewed
Jun 20, 2023
| 'postType', | ||
| 'wp_navigation', | ||
| { | ||
| per_page: -1, |
There was a problem hiding this comment.
Won't this need to be 100 to match with the PHP preloading? I think we have middleware on the client side that doesn't allow unbounded queries (i.e. -1) but worth double checking.
getdave
reviewed
Jun 20, 2023
|
I noted some potential things to consider before we merge this. Also the tests are going to fail until you update them to account for the change to include draft menus. |
|
Once this is merged we'll need to ensure the changes are included in WordPress/wordpress-develop#4628 |
|
I fixed the tests... |
getdave
reviewed
Jun 21, 2023
| @@ -8,6 +8,7 @@ import { store as coreStore } from '@wordpress/core-data'; | |||
| * Internal dependencies | |||
| */ | |||
| import useNavigationMenu from '../use-navigation-menu'; | |||
| import { SELECT_NAVIGATION_MENUS_ARGS } from '../constants'; | |||
There was a problem hiding this comment.
I know this might seem like a massive nit, but I would really prefer to avoid relying on app code to provide abstraction in tests.
Whilst it seems good now, further down the line it could lead to unexpected consequences with tests "automatically" adapting their behaviour when the production code changes.
I'd much prefer to see these arguments inlined and have a tonne of duplication which keeps things explicit and avoids abstraction in tests 🙏
Suggested change
| import { SELECT_NAVIGATION_MENUS_ARGS } from '../constants'; |
getdave
reviewed
Jun 21, 2023
Comment on lines
-47
to
+60
| per_page: -1, | ||
| per_page: 100, | ||
| status: [ 'publish', 'draft' ], | ||
| order: 'desc', | ||
| orderby: 'date', |
There was a problem hiding this comment.
I much prefer the inlining here 👍
…vigation-menus/constants.js Co-authored-by: Dave Smith <getdavemail@gmail.com>
|
Backporting in WordPress/wordpress-develop#4628 |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…1422) * Navigation: List draft navigations * Add a draft status to the navigation name * update the endpoints to make them more similar * Update packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/constants.js Co-authored-by: Dave Smith <getdavemail@gmail.com> * pacify PHPCS * update the entity to check * move the query config to a const * update tests * remove the abstraction for tests --------- Co-authored-by: Dave Smith <getdavemail@gmail.com>
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.
What?
List draft navigations as well as published ones in the navigation sidebar, so that the browse mode matches the block.
Fixes #51417.
Why?
So that the same list appears in both places.
How?
Change the preload query. The other alternative is to hide the draft ones in the block.
Testing Instructions
Check that the list of navigations in the sidebar in browse mode is the same as the list in the block inspector controls.