Navigation Overlay customisation via Template Parts #55657
Conversation
getdave
requested review from
scruffian,
richtabor,
MaggieCabrera,
ajlende and
jeryj
|
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
1 similar comment
|
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
|
Size Change: +2.03 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d8b1479. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7226914157
|
packages/block-library/src/navigation/edit/edit-overlay-button.js
Outdated
Show resolved
Hide resolved
TodoDumping a load of todos that are top of mind:
|
|
Here is how we can fetch the overlay in the PHP code $theme = 'emptytheme';
$custom_overlay_id = $theme . '//' . 'navigation-overlay-' . $attributes['ref'];
$custom_overlay_query = new WP_Query(
array(
'post_type' => 'wp_template_part',
'post_status' => 'publish',
'post_name__in' => array( 'navigation-overlay-' . $attributes['ref'] ),
'tax_query' => array(
array(
'taxonomy' => 'wp_theme',
'field' => 'name',
'terms' => $theme,
),
),
'posts_per_page' => 1,
'no_found_rows' => true,
'lazy_load_term_meta' => false, // Do not lazy load term meta, as template parts only have one term.
)
);
$custom_overlay_post = $custom_overlay_query->have_posts() ? $custom_overlay_query->next_post() : null;Then we can render it dynamically via |
|
Next step - break apart the rendering of the Nav block inner blocks so that we have x2 "modes":
Alternatively we might be able to configure the Responsive Wrapper to render different content depending on whether there is a custom overlay. But that might be causing it to know too much. |
Doesn't filtering the block edit component allow us to do this?
Did the idea of an overlay block fall off the map? Would this group be targetable from theme json by themes?
If the navigation block refs the same navigation cpt what is there to sync?
why do we want this? |
Yep probably. But we only want to filter if the block is within a specific Template Part. What data is there to access that provides this info? Some discovery needs to be done. For example, we might need to add post meta to the Template Part to denote it as a "overlay" so that we can filter the Nav block accordingly 🤷
Yes we decided we didn't need that. Indeed, we should avoid additional blocks if we can. However, if the problem above proves insurmountable we may need to revert to this approach.
The question raised by others was, what happens if the menu in the parent Nav block is changed? Currently in this PR a totally new overlay will be created based on the template part provided by the Theme. However, that may not be desireable.
Because that's the objective of the entire exercise 😅 The idea as I understand it, is that you could have totally custom content in the overlay. Therefore the rendering of the overlay needs to know how to render
This is in contrast to the current system which renders the exact same markup on all viewports. Literally it uses the same components - it's just displayed differently using JS and CSS. |
Because the content from desktop will not be the same as on the overlay once the user changes it |
|
Quick summary of how the classes at play for the block works today. We are using the same HTML for mobile and desktop, and we alter its appearance with CSS. This will need to change for our purposes here. For my own better understanding and anyone else's:
|
|
Although there is no necessity to maintain backwards compatibility, we want to try to preserve the markup of the block as much as possible to avoid breaking sites that rely on custom CSS for the navigation block. The markup when an overlay is present but has not been customized by the user will stay the same: And the proposed markup for when the user creates a custom overlay would be: The new customized overlay nav items will stay hidden until the menu is toggled. There are two questions this raises:
The navigation items would only be duplicated only when the user has created a custom overlay for mobile. /cc @WordPress/block-themers EDIT: A codepen with this mocked up: https://codepen.io/Marga-Cabrera/pen/GRzmxVR |
MaggieCabrera
added
the
Needs Accessibility Feedback
getdave
force-pushed
the
add/nav-overlay-as-template-part
branch
from
84cb759
to
918487f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/navigation-overlay.php ❔ lib/blocks.php ❔ lib/experimental/blocks.php |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
packages/block-library/src/navigation/edit/edit-overlay-button.js
Outdated
Show resolved
Hide resolved
That's why I would like to have the control not disabled, so it could be swapped easily to the desktop viewport. I would still think the navigation overlay would be the mobile overlay for the vast majority of use cases, even with interesting effects and motion. The overlay is always mobile, but rarely desktop. |
I'm not sure what the solution is, but as we allow alternate blocks in navigation, we quickly hit the edges of what's possible when it comes to styling. In my example, I wanted a background color on the overlay and my text + social icons to have a compelling color. The text color of the navigation is only applied within the overlay, as expected. But the social icon color is expressed on both the header and the overlay, which is unexpected - even if technically correct. |
I don't think there is any way around this. If you put a social links block in your Navigation and then that Navigation Menu is used in the overlay then the styles will inevitable be shared. I think with the advent of improved layout tools in Group, we should be encouraging folks to put social links outside of Navigation. Indeed, if you do this with your Overlay then you won't get any "inherited" styles. |
getdave
force-pushed
the
add/nav-overlay-as-template-part
branch
from
dae6262
to
a0c73c2
Compare
Not for the button, users should have to know they are breaking their site. I do not want the button for closing to be removable. Allowing an option even with a warning to trap users in modals is not something WordPress should provide an option for even if strongly discouraged. Thanks. |
Thanks for confirming. I would agree - this PR currently locking down that button. @alexstine Before this PR goes too much further, are you happy with the concept of hiding (I'm intentionally using |
Next StepsLet's try and get a working prototype again here. The criteria:
Try and keep Navigation parent ref and the ref in the overlay in sync. |
|
@getdave Not ignoring this intentionally. I was really backlogged when I returned from AWS re:Invent conference. Still trying to catch up. Hope to review this today. |
| <!-- /wp:group --> No newline at end of file | ||
| <!-- /wp:group --> |
There was a problem hiding this comment.
Looks like a linting thing?
| } | ||
| } else { | ||
| $content = $custom_overlay_post->post_content; | ||
| } | ||
|
|
||
|
|
||
| // Update the ref on the block | ||
| // if it doesn't have one, and turn off the overlay. |
There was a problem hiding this comment.
-
Update the ref:
To enable themes to provide a navigation-overlay.html. The theme won't know what ref to give the block so we have to pass it from the parent block -
Turn off the overlay
Navigation blocks inside an overlay can't also have an overlay otherwise we get into an infinite loop of overlays
|
@getdave Accessibility problem?
I do not see an Edit button next to that heading. |
| if ( overlay ) { | ||
| goToOverlayEditor( overlay.id ); | ||
| return; | ||
| } | ||
|
|
||
| // No overlay - create one from base template. | ||
| // TODO: catch and handle errors. | ||
| const overlayBlocks = buildOverlayBlocks( | ||
| baseOverlay.content.raw, | ||
| navRef | ||
| ); | ||
| const newOverlay = await createOverlay( overlayBlocks ); | ||
|
|
||
| goToOverlayEditor( newOverlay?.id ); |
There was a problem hiding this comment.
goToOverlayEditor is using history state, which isn't available on the Post Editor, so Clicking the Edit button on the Post Editor fails due to the undefined history state.
We'll need a different way to manage opening the Overlay Editor on the Post Editor.
There was a problem hiding this comment.
We'll need a different way to manage opening the Overlay Editor on the Post Editor.
Agreed. History doesn't work here. I suggest we disable overlay editing in the Post Editor and display a message about how you can access this. There isn't really a flow that will work.
| const onClick = () => { | ||
| if ( isSelected ) { | ||
| // Exit navigation overlay edit mode. | ||
| history.back(); |
There was a problem hiding this comment.
There are more routes available, so this doesn't work. Example repro:
- From the Site Editor
- Go to the Overlay Template Editor
- Click the Site Logo/W in the top Left to open up the left Sidebar
- Click the Editor Canvas
- Click the Overlay X
- The Editor Sidebar will open, and you can't exit the overlay edit directly back to the edtior
There was a problem hiding this comment.
Also, we should still have the Back button navigation in the central header area as on other templates.
There was a problem hiding this comment.
Also, we should still have the Back button navigation in the central header area as on other templates.
That's turned into a wider problem. I agree we need it but it will need to be a seperate PR.
UpdateI have posted (on the Issue) a set of what I believe to be the high level user requirements for this feature. |
| .wp-block-navigation__overlay { | ||
| display: none; | ||
| width: 100%; // fill the overlay | ||
| height: 100%; // fill the overlay | ||
| } | ||
|
|
||
| .is-menu-open.has-custom-overlay { | ||
| .wp-block-navigation__overlay { | ||
| display: initial; | ||
| } | ||
|
|
||
| .wp-block-navigation__default { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
We should comment this to explain what is happening here. Basically it's show/hiding the different content depending on whether it's being shown within the overlay.
getdave
force-pushed
the
add/nav-overlay-as-template-part
branch
from
6fdda8b
to
139d7cb
Compare
| @@ -57,6 +57,7 @@ | |||
| "@wordpress/private-apis": "file:../private-apis", | |||
| "@wordpress/reusable-blocks": "file:../reusable-blocks", | |||
| "@wordpress/rich-text": "file:../rich-text", | |||
| "@wordpress/router": "file:../router", | |||
There was a problem hiding this comment.
Router shouldn't be a dependency of the block library package as not every editor has one.
@glendaviesnz added a way to handle navigation in #57036 via an editor setting that will work across the post and site editors.
There was a problem hiding this comment.
Thanks for the pointer Dan 👍
There was a problem hiding this comment.
There may be some modifications coming to that approach now as we need to extend it a little probably in order to get rid of the template-only mode, will hopefully get that sorted today.
There was a problem hiding this comment.
the template-only mode
What is this mode you speak of?
There was a problem hiding this comment.
What is this mode you speak of?
It was added temporarily as part of the editor unification work, but we are now in the process of pulling it out again. Context being we wanted a way to edit a synced pattern in isolation in the post editor, so we looked at adding a pattern-only mode, but it was then decided that this was not very scalable as we didn't want modes for lots of different entities.
It was decided that a better approach would be to change the entity that was passed into the editor provider, and we added a getPostLinkProps editor setting which is a callback that returns an onClick event and href that changes the current entity in the editor.
We initially just used this for editing synced patterns in the post editor but I am now looking at doing the same for editing templates from the post editor, which currently uses the template-only mode. When working through this it seems our initial implementation doesn't extend quite so well for the wider use case, so I have a suggested refactoring of this here.
|
I posted an update regarding this PR on the Issue. |
|
For anyone coming here late the latest status is available. |
|
I'm rebasing this PR |
getdave
force-pushed
the
add/nav-overlay-as-template-part
branch
from
d8b1479
to
a4ccff6
Compare
| postId: overlayId, | ||
| postType: 'wp_template_part', | ||
| canvas: 'edit', | ||
| myNavRef: navRef, |
There was a problem hiding this comment.
| myNavRef: navRef, | |
| myNavRef: navRef, |
Let's rename this 🙏
|
I've rebased this and starting to take a look at it again after a long break. Here's what I'm thinking may help us to unblock this work a bit. Essentially we need to decouple when and how the Nav block receives a
We could continue with the ability of the Navigation block can function without a ref being set as an attribute ( a "temporary ref"), but still behave as though a ref is set. A block should be able to retrieve its "temporary ref" from various contexts including:
In addition, Navigation Overlay as an template part area feels a little over the top. It's extremely specific semantically - it's basically an area that exists for a single block only. That feels wrong. However, we can't easily have an "Overlay" area as that's too broad. Therefore we should look to find a different mechanism for identifying template parts that are "Navigation Overlays" (e.g. following a slug pattern, tags, post meta...etc). Todo
|
|
A template part / pattern still makes sense to me for this purpose. Perhaps we can model it after #61577 since it should be easy to unregister, replace, etc. |
🔧 List of outstanding tasks is here.
What?
Provides the ability to fully customise the overlay that is shown when the Navigation block is rendered on a smaller viewport (e.g. "mobile" phone).
Closes #43852
Alternative to this branch.
Co-authored-by: Ben Dwyer [email protected]
Co-authored-by: Rich Tabor [email protected]
Co-authored-by: Maggie [email protected]
Co-authored-by: Alex Lende [email protected]
Co-authored-by: Jerry Jones [email protected]
Co-authored-by: Andrei Draganescu [email protected]
Why?
Currently users only have minimal control over the contents and apperance of the overlay menu that is shown when clicking on the "hamburger" icon button which is shown when the Navigation block is rendered on smaller viewports.
This is very restricting and also very difficult to provide adequate control over because the exact same block (e.g. markup) is used on "desktop" layout and "mobile" layout.
Users have repeatedly requested the ability to
This PR seeks to address this requirement as an opt in experience, whilst also maintaining full backwards compatibility with the existing experience (😅).
How?
The premise of this PR is that the overlay should be a template part. This allows us to render the overlay's blocks within a dedicated editor (e.g. the same editor you use to edit your "Header" template part) and therefore provide full customisation of its contents.
At a high level it should work like this:
By default a new "Overlay" template part is created for each Navigation Menu (not block) only when the user chooses to customise the overlay.
The overlay template part will be created from a template part named
navigation-overlay.htmlmeaning the Themes can provide a default Overlay.Similarly, if the user creates their own "Navigation Overlay" template part within the editor then this will be used.
If a Theme does not provide an Overlay, then a default "Core" template will be used instead.
User Requirements
Please see the list of user stories on the Issue description.
Tasks
Editor(not required as we go to the template part editor instead 🎉 ).Xbutton can become a simple block which only has minimal options.Submenu & overlay text/backgroundoptions) on the Nav block (in parent and the block in the overlay). We should however, leave in theSubmenupart of the control as we still need to be able to Style submenus.Set the template part resizable canvas into a mobile viewport by default when this template part gets loaded.We decided against optimising for this for now.Ready for Dev
Displaypanel within inspector controls entirely for the navigation within the overlay.navigation-overlaytemplate file a.phpfile and implement translation of all strings.Defaultsetting for the Overlay's vertical padding which automatically takes into account the following variables that are outside of the "editor":Testing Instructions
Overlay Menu.Alwaysas this makes testing easier.OVERLAY MENU.Testing Instructions for Keyboard
Screenshots or screencast