Skip to content
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

Provide a better way to disable/enable nav block features for the nav editor #30007

Closed
Tracked by #29102
talldan opened this issue Mar 19, 2021 · 41 comments
Closed
Tracked by #29102
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@talldan
Copy link
Contributor

talldan commented Mar 19, 2021

What problem does this address?

The navigation editor currently uses a few 'hacks' to disable features of the navigation block that the navigation editor can't support. But also to add its own features.

These are (from least to most hacky) :

  • Using a block filter to add additional features (menu name editor)
  • Using a block filter to disable block supports features (pretty much all of them)
  • Using a block filter to remove variations (horizontal/vertical nav block)
  • Using a block filter to remove unsupported blocks from the inserter
  • Using a block filter to pass props into the edit function to disable some features. (item justification, submenu indicator, block placeholder)
  • Potentially using CSS to hide some parts of the block interface. (Navigation Screen: Update Navigation Block Toolbar to only include list view and menu name #28856 (comment))
  • Using CSS to completely re-style the block (accordion style submenus)

So this is a good opportunity to catalogue those and discuss if there's a way to improve a situation which will probably only become more difficult as the nav block receives more features.

What is your proposed solution?

Potentially more of these options could be moved to block supports.

Some of them might also be toggled by a theme.json setting, which the nav editor could more gracefully override.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block [Feature] Navigation Screen labels Mar 19, 2021
@talldan talldan added [Type] Discussion For issues that are high-level and not yet ready to implement. and removed [Type] Enhancement A suggestion for improvement. labels Mar 26, 2021
@draganescu
Copy link
Contributor

I would start by adding a supports:nothing supports 😆

On one hand it looks amazing from the enumeration above how flexible the block system is. Using the documented filter API blocks are quite flexible in cherry-picking what to use from their array of features, and also to expand their available features.

On the other hand the final two items of the list point to us needing another block. Is a block containing a promise of a certain UX for making kind of content? If so, when one needs to apply CSS to change the UX completely or to disable what is foundational block UX, then the promise is broken. Without this promise we get a new one, and hence a new block: the classic menu block.

Does moving the things we disable with filters to supports improve anything else other than dev ex? It may make a more performant navigation editor to not use filters I believe.

@getdave
Copy link
Contributor

getdave commented May 14, 2021

I took a brief look into this. The Block support mechanism can be used by doing a select hasBlockSupport from @wordpress/blocks store. The basically reads the values from the block.json supports field.

import { useSelect } from '@wordpress/data';
import { store as blocksStore } from '@wordpress/blocks';

const { supportsHorizontalOrientation } = useSelect( ( select ) => {
    const { hasBlockSupport } = select( blocksStore );

    return {
        supportsHorizontalOrientation: ! hasBlockSupport(
            'core/navigation',
            '__experimentalSupportsHoriztonalOrientation', // I made this up
            true
        ),
    };
}, [] );

The thing is that much of what we'd need to enable/disable is actually managed at a higher level than the block.

If we take orientation as an example. The way we "disable" the horizontal orientation option for the block in the Nav Editor is to manually pass vertical as the attributes.orientation value when we call createBlock(). In this case the user doesn't create the block at all - it's all programmatic.

As the hoz/vertical blocks are infact a variation, if we wanted to disable horizontal via supports then we'd need to remove the variations based on the supports. I'm not sure that's currently possible.

I wonder how many of the other features we need to enable/disable on the Nav Editor will have similar issues?

Perhaps a first step is to document all the features we currently disable/modify. Once we have these we can assess their suitability for the current block support mechanic.

@draganescu
Copy link
Contributor

Perhaps a first step is to document all the features we currently disable/modify. Once we have these we can assess their suitability for the current block support mechanic.

Yes, I think a concentrated list could show us some general approach?

@getdave getdave self-assigned this Jun 3, 2021
@getdave
Copy link
Contributor

getdave commented Jun 3, 2021

I'm going to start documenting all the things below. It will be updated as I go.

Documenting

Enhancements / Modifications

Item Method Notes
Menu name editor Filter on editor.BlockEdit Enables ability to edit the menu name in the Nav Editor only
Customising Nav block CSS styles Directly via CSS cascade Customises the layout of the Nav block when viewed in the context of the Navigation Editor.

Removing Features

Item Method Notes
Allow only Nav blocks in inserter Filter on blocks.registerBlockType Only allows the core/navigation and core/navigation-link block (plus variations) to be shown within the inserter. Effectively means non-Navigation blocks cannot be inserted in the Nav Editor.
Remove unsupported Nav block features Filter on editor.BlockEdit Disables various features of the Nav block that are not required/helpful in the Nav editor including justification controls and submenu indicators.
Remove unsupported Nav block settings Filter on blocks.registerBlockType Removes Nav block settings that are not required/useful within the Nav Editor such as custom classnames, HTML and block variations (eg: the UI that allows transformations to horizontal / vertical layout variations ).
Create vertical orientation Nav block Declaratively within call to createBlock Ensures the Nav block is created with a vertical orientation.

Other Considerations

  • If we utilise the block supports mechanism for enabling/disabling functionality then this will allow Theme authors to better customise how the Nav block is expressed in their Themes. For example they could choose which child blocks are supported.
  • We should not forget that there is a theme opt in that allows the Nav Editor to use both additional blocks (eg: social...etc) and block-based rendering. This affects how and when we disable/enable features.
  • Instead of defining custom CSS rules for the Nav block when used in the Navigation editor, could we instead register a block style and then set that as the applied block style in the Navigation editor?
  • We might be able to use theme.json to configure things on a per block basis. However overriding settings on a per-block basis is not something that Global Styles/Settings has considered. It's possible but should it be done 🤔?

@getdave
Copy link
Contributor

getdave commented Jun 9, 2021

Attempt to unify modifications via Block Supports (abandoned)

Let's proceed with a round of PoC PRs with the aim of taking as many of the items listed above and trying to enable/disable them via the Block Supports API. This will act as a feasibility study. If it's possible then we can use this one API to control all elements of the block.

Update: we abandoned this approach because the team felt that Block Supports was not the most appropriate API. We are now exploring Theme JSON.

@talldan
Copy link
Contributor Author

talldan commented Jun 10, 2021

Customising Nav block CSS styles

There were some other thoughts on this that might have been missed. One option is to make the nav editor's block styles become part of the block itself—I think @shaunandrews mentioned this could potentially be how vertical menus work by default. The nav block's submenus would need to be able to open using a click instead of a hover, which previously wasn't possible, but now should be. I think that's also tracked as a separate feature request.

Another thought I had on this is that maybe the nav editor shouldn't bother loading the nav block's styles at all. The problem that generally happens is a change to the block's styles causes a conflict with the nav editor's styles, so if they weren't loaded that wouldn't be an issue. Not sure if there are any caveats with this idea.

As the hoz/vertical blocks are infact a variation, if we wanted to disable horizontal via supports then we'd need to remove the variations based on the supports. I'm not sure that's currently possible.

I think the variations are already removed via the filter that modifies the block settings, which is not too bad, but the attribute is the thing that drives the orientation.

I have doubts over whether variations are the right way to go about exposing orientation anyway. So I don't think we should think about variations as a blocker. Potentially something to stop using and explore exposing the orientation UI in a different way.

Block supports could be interesting, it'd be good to get some wider input, maybe from @youknowriad and @mtias on this, as I this ties in with some of the layout concepts for blocks.

@jasmussen
Copy link
Contributor

The nav block's submenus would need to be able to open using a click instead of a hover, which previously wasn't possible, but now should be. I think that's also tracked as a separate feature request.

There's some conversation on how we might accomplish this in #18395 (comment). The best path forward isn't yet quite obvious, when for WordPress sites the top level menu item usually also links somewhere.

@getdave
Copy link
Contributor

getdave commented Jun 10, 2021

One option is to make the nav editor's block styles become part of the block itself

Yes I think this is the most logical and most maintainable solution with the caveat that those styles will still be somewhat divorced from the "Nav Editor" and still rely on someone not changing them without considering the impact on the Nav Editor.

That said, I did suggest we could make the Nav Editor styles a block style which would then make it an official style of the block without having to change the current default styles of the block. What do you think about that route @talldan? @draganescu was pretty keen I believe.

think the variations are already removed via the filter that modifies the block settings, which is not too bad, but the attribute is the thing that drives the orientation.

You're absolutely correct. However, I'd was trying to normalize all these enhancements/disabling via a single API (ie: Block Supports) so whilst it's not bad to be using another filter, it would be nice to normalize it.

So I don't think we should think about variations as a blocker. Potentially something to stop using and explore exposing the orientation UI in a different way.

I agree it shouldn't be a blocker for experimentation which is what I'm suggesting doing here. It means we'd need to have the attributes.orientation be constrained by a (hypothetical) allowedOrientations setting. However, would it go against the grain a little bit to have an attribute be overridden by a "supports" feature? Is there a precedent for that? Something to consider once the PR is ready.


@talldan I've now created proof of concept PRs to illustrate how we might be able to use Block Supports API to alter the features available in the Nav Block.

These are far from "polished" but they serve as a discussion point and also a means of evaluating the feasibility of the approach.

@talldan
Copy link
Contributor Author

talldan commented Jun 11, 2021

That said, I did suggest we could make the Nav Editor styles a block style which would then make it an official style of the block without having to change the current default styles of the block. What do you think about that route @talldan? @draganescu was pretty keen I believe.

I'm not sure about a block style, because the submenus need to be made to open on click, and JavaScript logic typically isn't coupled with block styles. But it could definitely be presented using a similar UI.

I agree it shouldn't be a blocker for experimentation which is what I'm suggesting doing here. It means we'd need to have the attributes.orientation be constrained by a (hypothetical) allowedOrientations setting. However, would it go against the grain a little bit to have an attribute be overridden by a "supports" feature? Is there a precedent for that? Something to consider once the PR is ready.

Some supports attributes declare an attribute, like classNames does here -

export function addAttribute( settings ) {
if ( hasBlockSupport( settings, 'customClassName', true ) ) {
// Gracefully handle if settings.attributes is undefined.
settings.attributes = {
...settings.attributes,
className: {
type: 'string',
},
};
}
return settings;
}

So the attribute could be moved from the block and handled by the supports implementation.

@getdave
Copy link
Contributor

getdave commented Jun 28, 2021

Please see #32779 (comment) where we discussed:

  • the scope of the Block Supports API.
  • the role Theme JSON could play in configuring block features/settings.
  • an alternative approach for removing the Nav Block as the wrapper and instead using InnerBlocks.

@getdave
Copy link
Contributor

getdave commented Aug 5, 2021

Suggested next steps with controlling the Nav Block and Nav Editor

I've been thinking a little bit about how we can best achieve the following objectives:

  1. Allow the Navigation block to be easily configurable by Theme authors in order to allow for "simple" and "complex" Navigations depending on what the Theme is designed/intended to support.
  2. Allow the Nav Editor screen to continue to utilise the Navigation block, but configured to be in a "simple" mode.

To do this I'm proposing the following:

  1. We re-engineer ("tweak"?) the Nav Block so that it's various features are controllable/configurable via Theme JSON.
  2. When in the Nav Editor screen, utilise the Theme JSON API (probably via hooks) so as to configure the Nav Block to be in a "simple" mode.

This is currently a high level indicator of the direction I feel we could pursue here so comments/feedback is welcome.

However, in order to get a clearer sense of next steps I propose the following tasks:

Once we have completed the tasks above, we should be able to implement a PoC whereby we take a Nav Block setting, make it configurable via Theme JSON and then use what ever mechanic is available to filter that setting on/off within the Nav Editor screen.

If that works then we can continue to update the Nav Block to be configurable via Theme JSON and consequently configure it to behave in a "simple" manner within the Nav Editor screen.

The end result should be:

  1. A Nav Block which is highly configurable and controllable by Themes without having to utilise complex hooks, filters...etc.
  2. A Nav Editor screen which can safely make use of a Nav Block which isn't changing underneath it thereby ensuring a consistent and intentionally minimalist experience.

Do we feel this is a good direction?

cc @draganescu @talldan @Mamaduka @gwwar @jasmussen

@jasmussen
Copy link
Contributor

We re-engineer ("tweak"?) the Nav Block so that it's various features are controllable/configurable via Theme JSON.

Regardless of the additional challenges that might be involved, I think it's just good form to make as much as possible configurable via theme.json. Just as a baseline, anything you could configure in the inspector should also be able to take defaults from theme.json. It's a code quality improvement that will also benefit themers.

As far as I'm aware, the biggest challenge to making that happen is that global styles output defaults with fairly low specificity (good in principle), whereas the navigation block outputs many styles with higher specificity. The good news is that we can use :where to lower specificity of rules. In #31878 I did that for margins and paddings, and the refactor in #33918 will make it profoundly simpler to expand that approach to other aspects that need it. More on :where here.

@talldan
Copy link
Contributor Author

talldan commented Sep 20, 2021

It's worth updating the issue here with the results from exploring #34784. Theme.json doesn't sound like the right path forwards. The block.json has been proposed has been proposed as an alternative.

Continually re-auditing (like we did in #30007 (comment)) seems like a good idea, because things have moved on since this was last assessed.

So here's a list of the way the navigation editor currently extends or modifies the navigation block:

Enhancements / Modifications

Item Method Notes
Menu name editor Filter on editor.BlockEdit Enables ability to edit the menu name in the Nav Editor only
Alternative block placeholder Filter on editor.BlockEdit Replaces the block's FSE style placeholder with a more detailed one for the navigation editor
Custom appender behavior Filter on editor.BlockEdit Makes the navigation block's appender still visible when the block is deselected
Customising Nav block CSS styles Directly via CSS cascade Customises the layout of the Nav block when viewed in the context of the Navigation Editor.

Removing Features

Item Method Notes
Allow only Nav blocks in inserter Filter on blocks.registerBlockType Only allows the core/navigation and core/navigation-link block (plus variations) to be shown within the inserter. Effectively means non-Navigation blocks cannot be inserted in the Nav Editor.
Remove unsupported Nav block features Filter on editor.BlockEdit Disables various features of the Nav block that are not required/helpful in the Nav editor including justification controls and submenu indicators.
Remove unsupported Nav block settings Filter on blocks.registerBlockType Removes Nav block settings that are not required/useful within the Nav Editor such as custom classnames, HTML and block variations (eg: the UI that allows transformations to horizontal / vertical layout variations ).
Create vertical orientation Nav block Declaratively within call to createBlock Ensures the Nav block is created with a vertical orientation.

Possible future features that require block modifications

@getdave
Copy link
Contributor

getdave commented Sep 20, 2021

Allow me to summarise where I feel we are at with this exploration.

My current take is that Gutenberg does not (yet) have any APIs that will allow us to systematically and reliably configure the Nav block in a way which accommodates all of its features as outlined by Dan.

Moreover, there is no way (that we've been able to uncover) to do this on a per editor basis.

Why?

Key Question

The question now is: given we don't currently have one, do we want to create a new API for controlling block specific features?. If so how do we see this working?

Potential outcome

If, for whatever reason, the answer to the question above is no then for the goal of controlling the Nav block within the Nav editor I would declare a programmatic approach a "no go".

In which case we must seriously consider abandoning the use of the Navigation block within the Nav Editor (to me just writing that seems strange as a concept) and instead begin pursuing @talldan's approach of a "Menu" block which would afford us total control of the block UX within in the Nav Editor.

Food for thought

Taking a step back, I feel that if I were a site/theme developer I would expect to have some unified mechanism that would allow me to programmatically control all the features of any given block. I would expect to be able to do this in any context, including between different editors. However, there clearly isn't one in Gutenberg.

@talldan
Copy link
Contributor Author

talldan commented Sep 20, 2021

I missed some of the comments here, so apologies for the late replies.

Also, have we considered creating a new navigation link container block just for Navigation Screen? It can be similar to the "Widget Area" block and only loaded in the navigation editor. I know this goes against the DRY principle, but it should make maintaining things a little easier.

I do think this is a valid option, and something I feel would solve a lot of problems. For some background, I created #30006 and #30007 to explore alternatives to that option, because it is quite a big step.

My concern is that there's still a large roadmap ahead for both the navigation editor and navigation block, and I expect their design and feature set to diverge significantly, mostly as the nav block implements new features and reconsiders new ideas for how menus can be built. These new ideas are hard to translate into the editor since it has to satisfy so many backwards compatibility concerns.

So what I feel might work is giving the user the choice between creating two types of menus:

  • A classic menu that strives to meet as much of the backwards compatibility criteria as possible. I'd recommend that this use a dedicated block type (as explored in Experiment: dedicated menu and menu item blocks for navigation editor #34496) so that the navigation block no longer has all of the concerns in this issue and maintenance is much easier. But that part is optional. This menu type uses the traditional way of storing and rendering menus.
  • A block-based menu that embraces the full navigation block and has very few backwards compatibility concerns. This would be the recommended type of menu if a theme opts-in to support it. This option would save and render block HTML and represents the future compatible option for users.

I think this model has benefits:

  • Users don't have to worry about backwards compatibility so much, the classic menu data is safe and won't be converted or forced to store block data.
  • Users are safe to explore creating block-based menus as a separate menu because there's no impact on existing classic menus.
  • Themes can provide editor styles for block-based menus so that the nav block is a more accurate representation of the front-end of a site.
  • Block-based menus can be seamlessly dropped into a block-based theme if a user decides to switch theme.

A few negatives:

  • Extenders now have two types of menus to extend instead of one.
  • Differing UI for creating menus between menu types.

@tellthemachines
Copy link
Contributor

The question now is: given we don't currently have one, do we want to create a new API for controlling block specific features?

Currently our only use case for such an API is being able to use the Navigation block in the Navigation screen. This is a super special case, where we're trying to leverage a block built with site editing in mind to basically redesign the classic menus interface. The problem with that is we have no idea what other use cases for this API might be, so it'll be very easy to end up with an API that is only useful for the Nav screen. It's better to wait until we have a few more scenarios where such an API might come in handy, and then design it based on common needs.

The Classic Menu block solution sounds good, especially given the precedent opened by Legacy Widget and Widget Group for editor-specific blocks to handle special cases 🙂

@getdave
Copy link
Contributor

getdave commented Sep 24, 2021

Currently our only use case for such an API is being able to use the Navigation block in the Navigation screen.

I don't think that's correct 😃 ...allow me to explain.

Let's say I work for an agency who develops websites for clients. Based on my own experience in this field, you will definitely want the ability to be able to lock down certain features of blocks in order to avoid clients being able to make decisions that you don't trust them with.

Currently we have a limited ability to achieve this via Theme JSON, but any block-specific features require hooks and filters to disable. In my opinion that's asking too much of a site developer or Theme creator. You should be able to declaratively control these features.

This is the use case I had in mind over and above the specifics of the Nav block/editor. It just so happens that (in my head at least) it chimed nicely with the scenario of providing a better way to control the Nav block in the editor.

I won't go through every block now and list out the block-specific features but I'm pretty sure they will be a number of them. Perhaps I'll come across them as I build out the block theme I'm working on?

Here's my final take: I'm pretty confident we'll see some mechanism (perhaps not a fully fledged API) for achieving this introduced into Gutenberg in the future.

@adamziel
Copy link
Contributor

adamziel commented Sep 27, 2021

@getdave @talldan If we want to configure the block depending on the context (nav editor / post editor / specific WP site etc), could we perhaps use block context API?

This is especially useful in full-site editing where, for example, the contents of a block may depend on the context of the post in which it is displayed. A blogroll template may show excerpts of many different posts. Using block context, there can still be one single “Post Excerpt” block which displays the contents of the post based on an inherited post ID.

Once a block has defined the context it seeks to inherit, this can be accessed in the implementation of edit

We could have sensible defaults in place to make the block work in the most common use case (e.g. post editor). The navigation editor would change these defaults by defining a context. We could even have a standard top-level component providing a context in all editors, and make it configurable via public API feature-by-feature (e.g. setBlockContext( "navigation", { placeholder: "detailed" } ) or as a bundle setBlockContext( "navigation", "navigation-editor" )).

I can't be sure if this was discussed already because of the sheer volume of comments. If it was, feel free to ignore.

Alternatively, I was wondering about leveraging editor settings here – I wonder if that was considered already.

@talldan
Copy link
Contributor Author

talldan commented Sep 30, 2021

@adamziel Editor settings I think was something considered before, but it's more for global configuration and less for settings relevant to a specific block.

Block context is interesting since that allows a specific consumer to use that data. I know the post editor provides the post id as globally available context, so there is the concept for a more global context, but post id is still very general compared to a specific feature on a specific block. Using context still feels very custom and not something that would ever apply to other blocks.

The challenge is there's no one solution for all of the things that the navigation editor does to the block. Right now, I'm thinking it's probably not worth worrying so much about where the navigation editor is adding features to the block. Filters are probably fine and the best we have right now. The navigation editor has control over these things and they've actually been pretty stable.

I think where the navigation editor is taking away features, this is more of a maintainability problem since newly added block features may need to be removed as well. Looking at the code of the nav block, this has already gone a bit wrong as the hasSubmenuIndicatorSetting prop is being used to remove three separate block features, not just submenu indicators.

It feels really difficult to determine what the right solution is for this. Maybe it's best to hold off on this for now, think some more, and see what other opportunities arise.

@adamziel
Copy link
Contributor

adamziel commented Sep 30, 2021

post id is still very general compared to a specific feature on a specific block. Using context still feels very custom and not something that would ever apply to other blocks.

@talldan @getdave Post id is very general, no argument here. At the same time, is there anything preventing us from giving contexts a new spin?

For me this idea feels very natural. We want different behaviors depending on the context in which that block is used. Even the word "context" comes up in these conversations all the time: "Nav block when viewed in the context of the Navigation Editor", "particular editor context", "context of the Navigation Screen". The information needs to come from outside of the block. Block context seems to check all these boxes, why not lean on it? What else could we implement that wouldn't ultimately boil down to passing contextual data to blocks?

Would it be nice to have a generic solution for fine-grained feature-by-feature control? Sure! But do we have to get there in the first iteration? I don't think we do. My understanding is that the minimal viable solution here would be a switch that says either "We're in the navigation editor" or "We're not in the navigation editor". It could be a contextual boolean flag.

Sure, that smells like tight coupling. But I think that's okay – these concepts actually are tightly coupled at the moment. I'd try to start small, see if we hit any limitations, and see where the iterative process take us.

@talldan
Copy link
Contributor Author

talldan commented Oct 1, 2021

@adamziel Another thought I've had about determining context is that the navigation block could determine its capabilities via the entity it's being used in. In FSE and the post editor it should be surrounded by either template part or post entities, and that should be available via an EntityProvider.

In the menu editor there wouldn't be a parent entity to the menu, we're in an isolated mode where only the internal data is being edited, so maybe this could be the thing that determines whether those styling capabilities are available.

@tellthemachines
Copy link
Contributor

If we want to configure the block depending on the context (nav editor / post editor / specific WP site etc), could we perhaps use block context API?

We're already using context in the Navigation block to define pretty much all its custom behaviour!

If we go with a solution like <!-- wp:navigation {"id":"menu","isResponsive":true,"fontSize":"medium"} /--> (taken from here, where the Nav block is a container with all its customization, and it renders whatever menu corresponds to the id it's given, then the current setup where we pass context from the block down to its children should just work as it is.

Wouldn't it just be a case of the Nav editor only using the menu content, and not the Navigation block wrapper itself?

@draganescu
Copy link
Contributor

What am I missing, as I think in this <!-- wp:navigation {"id":"menu","isResponsive":true,"fontSize":"medium"} /--> setup there is no need for the issue to exist anymore?

The wp:navigation simply gets configured via these attributes we pass to it. And that is OK because if we optimize for the "links only" or "simple menus" as the default behavior of the navigation block, the differences between classic and block themes are minimal, completely manageable via a few attributes to turn off some items. I think we wouldn't even need filters anymore.

@talldan
Copy link
Contributor Author

talldan commented Oct 4, 2021

What am I missing, as I think in this setup there is no need for the issue to exist anymore?

It's fine to be able to initialise the navigation block with some defaults for attributes. That's something that the editor already does. But the goal is to stop the user interface for these useless settings from being shown to the user. I don't see how that helps without adding extra attributes like canModifyResponsiveness.

A different take on it could be for the navigation editor to modify the block type of the navigation block and remove those attributes. The block itself could check it's own block type for existence of that attribute definition.

Wouldn't it just be a case of the Nav editor only using the menu content, and not the Navigation block wrapper itself?

That was discussed there, and possible, but it seems against the spirit of that comment you linked to. I read it as an argument against the idea of removing the wrapping block in the navigation editor.

@draganescu
Copy link
Contributor

I don't see how that helps without adding extra attributes like canModifyResponsiveness.

Yeah, apparently if block supports and theme settings are for other things, then block behavior configuration should be handled by attributes. Wether behavior control will be as granular as your example or result from a combo of other atributes will result on a case by case basis.

At least that is my current understanding.

@adamziel
Copy link
Contributor

adamziel commented Oct 4, 2021

Another thought I've had about determining context is that the navigation block could determine its capabilities via the entity it's being used in. In FSE and the post editor it should be surrounded by either template part or post entities, and that should be available via an EntityProvider.

@talldan In that scenario the block would decide based on the surroundings. Changing the behavior in a specific editor would mean updating that block, not the editor. New editors introduced via plugins wouldn't have that power. I'd rather put the editor in charge.

@adamziel
Copy link
Contributor

adamziel commented Oct 4, 2021

Yeah, apparently if block supports and theme settings are for other things, then block behavior configuration should be handled by attributes. Wether behavior control will be as granular as your example or result from a combo of other atributes will result on a case by case basis.

@draganescu Such attributes would only affect the editor and not the frontend. At the same time, they would be stored in with the block and presumably used regardless of which editor that saved block is edited in.

@adamziel
Copy link
Contributor

adamziel commented Oct 4, 2021

@tellthemachines

We're already using context in the Navigation block to define pretty much all its custom behaviour!

That's neat! I just saw this:

const { showSubmenuIcon, openSubmenusOnClick } = context;

Wouldn't it just be a case of the Nav editor only using the menu content, and not the Navigation block wrapper itself?

That's what I'm thinking, it looks like a relatively small change too.

@talldan
Copy link
Contributor Author

talldan commented Oct 5, 2021

Is there a need to use context if there's no parent block in the navigation editor?

That's what I'm thinking, it looks like a relatively small change too.

I experimented with this when I made this comment - #34496 (comment). Here's the branch, though it's quite outdated now:
https://github.com/WordPress/gutenberg/compare/experiment/keep-nav-link-but-drop-nav-block?expand=1

It's reasonably small JS change. but quite a big CSS change, as the nav block provides a lot of styles that the editor depends on. Could always copy/paste/rename as a first step though, before refactoring them properly.

The bit that we miss by losing the navigation block is all the discussed changes to how menus are saved - #34496 (comment). The nav editor would need to reimplement that.

@tellthemachines
Copy link
Contributor

It's reasonably small JS change. but quite a big CSS change, as the nav block provides a lot of styles that the editor depends on. Could always copy/paste/rename as a first step though, before refactoring them properly.

This would be a great excuse to remove block styles entirely and style the editor independently, as discussed as an alternative to #35149 😁

The bit that we miss by losing the navigation block is all the discussed changes to how menus are saved

If we store nav block contents as custom post types, can't we still use and modify those contents? I thought the whole idea of a CPT was being able to decouple the wrapper from its innards.

@talldan
Copy link
Contributor Author

talldan commented Oct 5, 2021

If we store nav block contents as custom post types, can't we still use and modify those contents? I thought the whole idea of a CPT was being able to decouple the wrapper from its innards.

We still lose the navigation block's implicit system and have to reimplement it. The same would be true of the the direct insert / default block logic that was just added.

Not an argument against the idea, just pointing out the trade-offs.

@adamziel
Copy link
Contributor

adamziel commented Oct 5, 2021

Hm maybe we're diverging into another direction here. Using contexts to configure the navigation * blocks is one idea, and removing the root navigation block in some contexts is another. Wrapper or not, the inner blocks still need that configuration bits. I'll start a PR to explore that a bit.

@adamziel
Copy link
Contributor

adamziel commented Oct 5, 2021

Here's a proof of concept – I'm interested in all your thoughts: #35351

@mtias
Copy link
Member

mtias commented Oct 12, 2021

I see the work on #35418 more aligned with the nature of blocks and block parents. Generally, we should not have a need for context that doesn't come from internal heuristics (i.e. the nature of the blocks placed within) or the configuration of the parent (attributes of various kinds). There's always a parent, whether implicit in the root of an Editor provider or in a more explicit wrapper like the template part holder in the case of block themes. We can model that however is easier to support and maintain, but the flexibility should be inherent to the blocks that participate.

@draganescu
Copy link
Contributor

I think we can close this as the stand alone navigation editor work has been replaced with incoming work related to a focused mode (template part like) editor for navigation blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

No branches or pull requests

9 participants