Fetch classic Menus and Pages using view (readonly) context #37884
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: +7 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
getdave
commented
Jan 11, 2022
packages/core-data/src/entities.js
Outdated
| @@ -205,7 +205,7 @@ export const prePersistPostType = ( persistedRecord, edits ) => { | |||
| * @return {Promise} Entities promise | |||
| */ | |||
| async function loadPostTypeEntities() { | |||
There was a problem hiding this comment.
FYI these are the changes from #37685
|
I am not seeing any error in the network tab, which is good. But the dropdown is empty here. Am I missing something here? |
That's expected. It's easy to miss. Here's what I wrote in the PR description:
|
Okay, if that is the case, then the dropdown should be hidden or something. A empty dropdown looks weird / broken. |
Oh I see. Looking again this morning with a less tired brain I see that's a bug. The
|
|
The perfect solution here, would be, if you are a contributor, show a list of classic menus and allow you to create a navigation block, but save the data to the post content. What is the point of making the api request and to be able to see a list of classic menus and not be able to do anything about it... |
I agree that we may need to introduce a mechanic that allows us to save Nav blocks to the post content rather than the CPT. This will be required for example, when we start building more complex "mega menus" that may not be suitable to be saved without presentational attributes and thus won't be designed to be transferable across Themes.
Well indeed. Ideally we want to avoid unnecessary API requests and it should be possible to do so. |
8 tasks
|
I've addressed the UI issue of showing/hiding the |
|
I have approved #37980. Once this is merge and this PR is rebased, I will re-review this. |
|
Bear in mind we cannot merge this until @youknowriad merges #37685 |
getdave
force-pushed
the
try/nav-block-read-menus-using-view-context
branch
from
08e31b7 to
db43ac7
Compare
@spacedmonkey This PR has now been rebased. |
getdave
force-pushed
the
try/nav-block-read-menus-using-view-context
branch
from
db43ac7 to
0313d8f
Compare
|
This PR looks good. It is a nicpic, not sure it should be part of this PR. But the is a random line here after navigation. Otherwise this PR looks good. |
Yeh fair enough. It's not conditionally shown because - I guess - we never intended such a state to exist. Indeed I wonder whether we ought to have some inline message here? Seems like a confusing state. If not then I'd suggest we raise a follow up PR to only show his |
|
I have created a follow up ticket, for allow contributor's to create navigation blocks. See #38030 |
spacedmonkey
approved these changes
Jan 17, 2022
|
@spacedmonkey did you want this backported for the RC? |
|
@getdave i do think it should be yes |
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.
This PR builds on #37685 and acts as a test case for that PR.#37685 is now merged so we can proceed here if ✅Description
On
trunkwe are currently attempting to "read" data about Menus and Pages but we are using theeditcontext to do so. Therefore when you are a lower permission user (e.g.Contributorrole) this results in a 403 error because the user doesn't haveeditpermissions.This PR follows what @youknowriad suggests which is to adjust the
contextparameter on the call togetEntityRecordsto beview. This means the request will succeed and the data can be read.Note this test case is rather limited.
How has this been tested?
Compare this to
trunkwhere you will see 403 errors.Note that the UI won't change because only
Adminrole users are allowed to create Menus and thus access to creating Navigation Menus from either Classic Menus or Pages is not exposed in the UI.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).