-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FSE: Add transform + upgrade nudge from A8c Nav Block to Core Nav Block #38197
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
@shaunandrews Heads up that I'd appreciate a review on this just as soon as I have a working implementation. It could be tricky to test so it might need to be screenshot based unless you have a working FSE sync setup (see |
Feature MappingFont Size
There is no equivalent on the new block. Text Align
This feels like the closest match. Align
Alignment should map 1:1 with the setting on the new Block. The only oddity is that the old Block supports Text Color
We don't support text colour which is a problem... Background Color
We don't support background colour. We can however, wrap the Block in a Group block and apply the setting there. Might be a little odd though? We'd also have to account for mirroring the alignment settings on the Group Block (as happens when you "Group" using the toolbar). Text Color
We don't support text colour which is a problem. |
Let's make sure all attributes are mapped correctly, too, including block alignment please |
70aee31
to
301a3a3
Compare
Agreed. That's what I was meaning by the features really. See above. |
<p> | ||
<span className="posts-list__message"> | ||
{ __( | ||
'An improved version of the Navigation block is available. Upgrade for a better, more natural way to manage your Navigation.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to extract the notice to a separate component / PR. It already gets duplicated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. If this is something that can be shared across FSE then that's a good idea. I'd be inclined to ship "as is" and then unify in a separate PR.
Any chance you (or someone) can add screenshots? |
There's some spacing issues, and overall the message is way too verbose. Lets try something like this:
I think we should avoid the term "Upgrade" as it could have confusing implications related to upgrading a paid plan. I'd love to know what @obi2020 thinks here, as well. |
Big +1 for using If there's something concrete like "This version will stop working..." or "Upgrading will add [this functionality / performance improvement]" Are there any repercussions if they don't upgrade? |
Great question. I'd assume we'd want to remove the old block, but I'm not sure its realistic. I also don't think we could explain the value in updating to the new block in a few words, which is why I tried to just say "improved block" and hope people click the button. |
@shaunandrews @obi2020 Thanks for feedback.
Correct, we can basically never remove this Block now or at least until Gutenberg Core offers an automated way to migrate between different Blocks. Even then it might not be a good idea. The current wording for the Notice has something to indicate that the upgrade may not result in a 1:1 experience visually. There's no way for us to guarantee the same results. Happy to amend it to whatever we feel is best. |
🛑 Update - Blocking Issue - cannot accurately map Nav Items between BlocksThis PR is currently blocked and unable to move forward. This is because of the following problem. Navigation is comprised of a set of navigation items. Upon "upgrade", we should be mapping these 1:1 between the two blocks. To understand why this isn't possible, we need to understand how the two blocks differ in their approach to determining which Nav Items are included in the Block. The existing a8c block uses server-side render of the Navigation. It uses If this isn't found then it falls back to using By contrast, the new Core block creates nav items from top-level Pages retrieved via the REST API. It does not look at WP Menus because there is not a current REST API endpoint for Menus. Therefore we have a scenario where a user could have created a WP Menu on their site via the Customizer. With the old a8c Block this Menu would be what is rendered as the Nav Items. If that user then upgrades to the new block then they would see (potentially) different Nav Items. This is because the new block doesn't take account of their custom WP Menu but rather just adds all top-level pages. This means the user loses their carefully crafted navigation. Finally, it's important to note that transforms cannot be async. Therefore even if we have the Menus endpoint we cannot call this during a transform to auto create the nav items in the target block. A potential solution might be to do the transform, insert the block and then use some attribute on the inserted block to auto-create from Menus instead of going to the placeholder state. This requires investigation however. @apeatling @obenland This issue is currently blocking. I see a few possible options to allow this to move forward:
If there are other options I'm not aware of then I'd be happy to hear them. I would say that none of the above are simple and its likely this runs past the end of our Iteration. As things stand, I would greatly value a discussion of options and direction on next steps. Many thanks. UPDATENote as of this comment, the Block is now also able to parse and use WP Menu items as well so the issue above may no longer be a problem. |
An update. Having discussed with @marekhrabe we've looked at our options. We've come up with trying another route which is to pase a inlined JSON object alongside the server side rendered Nav Menu response. We can then parse the JSON out of the DOM generated by the SSR and use that to create the Nav items during the transform.
Done. POC in place. |
@shaunandrews @obi2020 I've updated the visual spacing and text. Also made some tweaks as I felt necessary |
The visuals look better. I'll repeat what I said previously about the use of the word "Upgrade":
I still think the message is too long, and can be summed up in a single sentence:
|
@shaunandrews Sorry just after I did that screenshot @marekhrabe reminded me about Update vs Upgrade. That's been amended now 👍 |
@obenland curious why you believe this one isn't a blocker? |
The Core block doesn't offer background or fonts size customization so I'm not sure how we'd keep that the same without adding that support. Font color I'd expect to be mappable? |
This mirrors the behaviour of the rendered Nav Menu fallback `wp_pages_menu`
ef8a0d9
to
ef68957
Compare
Good spot @obenland. I tested on my Dotorg install and turns out I was missing two guard clauses. Now we guard against empty JSON response in markup and also always ensuring we return some data even if there are no menu locations. All fixed. |
I'm not familiar with the block upgrade flow, but I wonder if, to work around the missing background color, we could wrap the Nav block with a Group block with the background color attribute. Though, we'd still be missing the text color, which might prove problematic if the user, for example, "inverted" the Nav colors. E.g. |
I've tracked down that the many of the styles on the a8c Nav Block are being applied via the Theme styles (eg:
Update: this was wrong. The Core Nav Block now has text color support. So we just need to see if the Group Block route works for us. Therefore, I feel we need direction from @apeatling on whether we:
See also @Copons 's point about usage in current Themes:
Lastly, bear in mind we still need to add support for nested levels of Nav items during the transform so this isn't ready to go yet anyway. |
@Copons Sorry to delay response to your thoughtful comment. Please allow me to address:
I think @apeatling confirmed that the inability to transform Navs created from WP Menus would be considered a Blocker. However as you indicate it is extremely unlikely this will ever have occurred (not your fault but I wish I'd had this insight earlier). The work to solve this has been completed now anyway so it doesn't matter. Moreover, it was partly necessary to allow us to achieve the "auto create" functionality and bypass the placeholder state on the Core Nav so it wasn't all wasted time.
This is a good point. Indeed I've been using this mechanic for testing. However it doesn't set a good precedent if you upgrade on a prompt given by us only to find a completely substandard experience. As a user once I've been "bitten" by such an experience am I ever likely to update any of the other blocks? Probably not. Therefore I think it's worth getting this right.
I think the only way we can know for sure is to test against usage in Themes.
Agreed. This will be important for @apeatling and @obenland to know. |
UpdateWe're going to put this on hold whilst @apeatling considers next steps and whether we can enhance Core to gain feature parity with the a8c Nav Block. |
Next steps after discussing with the team:
Relevant: WordPress/gutenberg#19108 |
I am closing this in favor of the current efforts with core FSE. |
@getdave This still has merit, right? We still want to deprecate the custom navigation block? |
@obenland feel free to reopen as needed! This was closed as part of bug triaging we are doing. |
I think so. I think we'll need to do quite a few things to facilitate the transition to the core site editor from dotcom FSE, so it could be worth waiting until then to do all the work |
Gutenberg now has a native Nav Block
core/navigation
. As a result we can start to deprecate thea8c/navigation-menu
block from FSE.To do this we are going to add a transform to the a8c block and then prompt the user to run the upgrade manually.
For more see
paYJgx-lF-p2
Screenshots
Testing instructions
It is...complicated.
paAmJe-Nv-p2
and have the sync from FSE to Dotcom up and running.npx lerna run dev --scope='@automattic/full-site-editing' --stream
to watch and build FSE files.Alves
FSE Theme.Page
.Header
template part.core/navigation
Block.