Flag invalid Navigation Link items. #31716
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
vcanales
changed the title
vcanales
force-pushed
the
invalid-navigation-items
branch
from
7cd7744 to
8a0e0d7
Compare
|
Size Change: +645 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
This is cool:
I pushed a small padding change:
I think we can potentially revisit and refine the menu item placeholder style as we go, but that's something I'll be looking into separately. This just needs a code review from what I can tell! Nice. |
|
Thanks for taking a look. These changes broke the e2e tests, which I think indicates that some of those tests depend on invalid links showing up normally.
Agreed, I'd appreciate a look from @WordPress/gutenberg-core |
ntsekouras
reviewed
May 19, 2021
vcanales
force-pushed
the
invalid-navigation-items
branch
from
9a5eaef to
81eb6c7
Compare
ntsekouras
force-pushed
the
invalid-navigation-items
branch
from
81eb6c7 to
63493a2
Compare
ntsekouras
reviewed
May 25, 2021
There was a problem hiding this comment.
I rebased but the Navigation failing tests might be relevant or related to core changes 🤔 . Needs investigation
One thing that we might take into account is that while the link is updated to invalid link, in ListView is still shown as before. Should we handle this views as well?
vcanales
force-pushed
the
invalid-navigation-items
branch
2 times, most recently
from
ea14935 to
5982a89
Compare
vcanales
force-pushed
the
invalid-navigation-items
branch
from
5982a89 to
ff86d7a
Compare
gwwar
reviewed
Jun 8, 2021
There was a problem hiding this comment.
Overall, this looks really solid! Nice work @vcanales ✨ As discussed in chat it looks like e2e mocks need to be updated to handle the new getEntityRecord call to fix tests.
One usability question I had was around how do folks know why this is invalid and how to fix it? Current workflows allow us to create drafts within the search suggestion, so seeing this from the start may be surprising.
draft.mp4
vcanales
force-pushed
the
invalid-navigation-items
branch
from
ff86d7a to
db5f71d
Compare
vcanales
force-pushed
the
invalid-navigation-items
branch
from
db5f71d to
f0f131c
Compare
There was a problem hiding this comment.
Thanks for working on this! A couple minor questions below, and some thoughts about the Draft status:
The current menus screen only flags deleted posts/pages as invalid. Drafts are kept in the menu:
The front end behaviour also differs with current menu showing draft pages (though they link to a 404 unless user is logged in/able to access the draft), while the Navigation block hides them.
I think unless we have really good reasons to change this, we should follow current behaviour so users know what to expect. This would also help solve @gwwar 's point about creating draft pages in the Navigation block.
vcanales
force-pushed
the
invalid-navigation-items
branch
from
3c21206 to
ad6df50
Compare
vcanales
force-pushed
the
invalid-navigation-items
branch
from
ad6df50 to
7777d4d
Compare
jasmussen
approved these changes
Sep 6, 2021
vcanales
force-pushed
the
invalid-navigation-items
branch
2 times, most recently
from
5d18b97 to
dda0fb7
Compare
|
Worth noting that if you are using the menu item request rest api endpoint, there is a field called |
13 tasks
vcanales
force-pushed
the
invalid-navigation-items
branch
from
dda0fb7 to
95ec8f3
Compare
vcanales
force-pushed
the
invalid-navigation-items
branch
2 times, most recently
from
2934840 to
579700d
Compare
vcanales
force-pushed
the
invalid-navigation-items
branch
from
579700d to
45e19e0
Compare
Necessary as the invalid links logic calls `getEntityRecord` on pages, and if they don't exist a 404 error is thrown, causing tests using `mocklSearchResponse` to fail, as the pages being looked up don't exist. Using `createPages` does not fix this issue.
vcanales
force-pushed
the
invalid-navigation-items
branch
from
45e19e0 to
bad275f
Compare
spacedmonkey
reviewed
Feb 4, 2022
This was referenced Aug 3, 2022
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.
Description
Re. #23573
Following the criteria used when deciding whether or not to render a Navigation Link item server-side, mark those items that would not be rendered as Invalid in the editor, in order to match the behaviour of the menu editor found on
nav-menus.php.The steps taken to decide whether or not an element is invalid are the following:
If those conditions are met, check if
If either of those is true, invalidate.
How has this been tested?
Manually tested by:
Both modified pages should now be flagged with the text "(Invalid)".
And
All Drafts and trashed posts should be flagged with the text "(Invalid)".
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).