Standardise and fix parsing and serialization of blocks and nav menu items - #31004
Merged
Conversation
getdave
added
[Block] Navigation
|
Size Change: -185 kB (-12%) 👏 Total Size: 1.31 MB
ℹ️ View Unchanged
|
getdave
commented
Apr 20, 2021
talldan
reviewed
Apr 21, 2021
Contributor
Author
|
I've realised this is more complex than I thought. Essentially we have x2 fields on the
We also have On our
I think we need to update everything as follows:
I'll dive into this again tomorrow. |
Contributor
Author
|
Watch out for
|
Contributor
Leaving a brain dump of the current values. I think all props are optional (we render placeholders in the editor). Some older data might be missing
gutenberg/packages/block-library/src/navigation-link/block.json Lines 1 to 35 in ee84e94 |
Contributor
|
I think we have some flexibility in the data formats, what might help drive this is adding unit test cases for our common scenarios. If it makes sense, I think we'll want three suites:
Each suite would likely cover cases like:
|
getdave
force-pushed
the
try/create-nav-link-variations-from-menu-data
branch
from
April 23, 2021 12:20
bddea9f to
6e726cb
Compare
getdave
marked this pull request as ready for review
April 23, 2021 12:22
getdave
requested review from
adamziel,
ajitbohra,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
April 23, 2021 12:22
getdave
changed the title
7 tasks
This comment has been minimized.
This comment has been minimized.
…field. This field is the correct field in which to store the type of the object being stored (eg: “post”, “pages”…etc). See https://core.trac.wordpress.org/browser/tags/5.7.1/src/wp-includes/nav-menu.php#L796
Contributor
Author
|
Ok @gwwar - thanks for your reviews and comments. Super helpful and much appreciated. I've revised everything now and I think it's all working better. The only things I'm not sure we need to cover in this PR are:
These seem to both be existing issues and we'd be growing this PR very large if we tackle them here. Happy to raise and follow up immediately afterwards. Will ping again in the nav editor Slack channel for review. |
talldan
reviewed
Apr 30, 2021
Contributor
There was a problem hiding this comment.
Looks really good so far, it's definitely nice to see this code move toward something more maintainable (and correct).
I did some testing by saving various types of menu items in the current menu screen, then loaded up the new nav screen, and then the reverse of that process.
There are a couple of things I've noticed:
- A tag created in the current menu screen becomes a custom Link when loaded in the new nav screen
- A label edited in the new nav screen isn't saved, loading up the current menu screen Custom Link labels are deleted and changes to other types aren't saved.
I didn't read all of the previous comments, so that may have been mentioned already.
Contributor
Author
✅ Fixed Screen.Capture.on.2021-04-30.at.13-20-29.mp4Good catch. Another one which @gwwar also noticed. I've been through again and I was able to replicate. The issue is that the block variation is I've decided to avoid a block migration by simply doing the conversion on the fly when mapping blocks -> menu items (and vica versa). I've added x2 specific tests to provide coverage on these and documented the reasons behind the decision in code.
✅ Fixed Screen.Capture.on.2021-04-30.at.13-21-52.movGot it! I think @gwwar picked this one up as well. I've just worked out the cause, fixed the issue and added some test coverage. |
Contributor
Author
|
@gwwar @talldan I looked into the Other than that I believe all the previous points have now been addressed, namely: ✅ Updated to use per block/menu item transforms rather than per property. So the only issue left is I'd say I'm ready for a final review 🤞 🙇 |
gwwar
approved these changes
Apr 30, 2021
Contributor
There was a problem hiding this comment.
Nice work @getdave this is a huge improvement on our serialization and data handling in the navigation editor! ✨
I'm giving tentative approval. ✅ Please fix the opensInNewTab case before merge. No need for another look from me for that case if that's sorted.
I tested this by some manual smoke testing starting from various editors, and seeing if values were persisted starting from one editor or another.
Here's a quick example case:
serialize.mp4
gwwar
reviewed
Apr 30, 2021
Merged
20 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Aims to fix #29793.
This PR does two things:
core/navigation-linkblock variations based on the menu item data from the database.core/navigation-linkblock variation.Postor aPage...etc).Known Bugs
Note in the process of working on this PR I've uncovered a few more bugs which I won't attempt to fix in this PR lest it get too large. I'm noting for future reference here:
Post linkin the block sidebar (screenshot). They should probably use the title of the Custom Post Type (eg:Portfolio link)Post Type Archiveas a result these are saved asCustom. We need a new variation to be created. Here's how you add aPost Type Archiveto a menu. See Add Post Type Archive variation tocore/navigation-linkblock #31452Other attributes aren't always parsed/serialized correctly from block -> menuItem and menuItem -> block (eg:Update: this is now resolved as part of this PR.attr_label...etc). Once this PR is merged, we need to audit these and write some automated tests to ensure they are adding to the mapping for conversion.Custombecause the REST API endpoint forv2/searchdoesn't return numeric IDs for the post formats. See Fixpost-formatserialization due to invalid REST API response foridprop #31380How has this been tested?
Setup
Test
core/navigation-links should appear with the correct block variation (eg:Post Link,Page Link,Custom`...etc. There are a few known exceptions that don't work which - given that they have never worked - I will tackle in a follow up. These are:Post Type Archivethis will not work because we don't have a variation setup for this yet.Portfolio Categorieswill show up with the block variation name beingCategory. This needs fixing.Screenshots
Menu -> Nav Editor -> Menu
Screen.Capture.on.2021-04-23.at.13-02-18.mov
Menu -> Nav Block
Screen.Capture.on.2021-04-23.at.13-05-53.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).