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

Add navigation menu blocks #14856

Merged
merged 2 commits into from Jun 18, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented Apr 6, 2019

Description

This PR adds navigation menu blocks it is a first small step to get #13690.
It is a first small interaction using only what blocks can do right now.
It still contains the nested blocks UI.
But it creates a basis from where we can follow up in parallel.

Short term follow up tasks to get to the v1:

  • Finish the inserter component as shown in #13690.
  • Add a logic that when a navigation menu is created we automatically generate a block list based on top level pages.
  • Allow InnerBlocks to be customized so we can remove the wrapping block UI.
  • Expose block Drag & Drop/mover functionality so it can be customized/reused here.

Changes outside block-library done here are small improvements to make this possible but they are not related to this PR and will be extracted and properly documented.

How has this been tested?

I checked it is possible to create a menu, I checked the output markup is the expected one.

@gziolo
Copy link
Member

left a comment

It would be nice to follow #14770 and introduce block.json file for new blocks.

Show resolved Hide resolved packages/block-library/src/index.js Outdated

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from 1b083fb to f20fb07 Apr 8, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch 2 times, most recently from c3ac43f to 316b902 Apr 8, 2019

{ __( 'Don\'t let search engines follow this link.' ) }
<ExternalLink
className="wp-block-navigation-menu-item__nofollow-external-link"
href="https://codex.wordpress.org/Nofollow"

This comment has been minimized.

Copy link
@Soean

Soean Apr 9, 2019

Member

Maybe add a i18n function?

@gziolo
Copy link
Member

left a comment

How about we put this block behind the GUTENBERG_PHASE feature flag and merge it later this week? @mapk how does it look behavior wise?

Aside: Reviewing 750 lines long PR is going to be suboptimal if we continue iterating too long on this PR.

@Soean Soean added the New Block label Apr 9, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from 316b902 to 5a85173 Apr 9, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Hi @gziolo all your feedback was applied. I was in doubt if I should use hooks given that we are not using them widely on our code base. But I think it is time to start and given that we are implementing totally new blocks it seems a nice opportunity. I refactored the code to use hooks and simplified it.

@gziolo
Copy link
Member

left a comment

Code wise, it looks great. I love how those React hooks make everything simpler. After all, it isn't that much to learn after you do the homework by reading their docs and the article I referenced in my previous comment.

Love your latest changes ❤️


edit,

save( { attributes } ) {

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

The direction is to have all client-side specific cod in its own file. save should be moved to save.js file. This will allow to get rid of index.js completely at some point.


edit,

save() {

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

The same note about save as for another block :)


description: __( 'Add a page, link, or other item to your Navigation Menu.' ),

supports: {},

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

It looks like it can be removed.

Show resolved Hide resolved packages/block-library/src/navigation-menu/menu-item-inserter.js
@@ -103,6 +105,8 @@ export const registerCoreBlocks = () => {
process.env.GUTENBERG_PHASE === 2 ? legacyWidget : null,
missing,
more,
navigationMenu,

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

Can you put those blocks behind the feature flag similar to Legacy Widget?

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from 5a85173 to 3a4e3e2 Apr 12, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Thank you for the code review @gziolo! The feedback was applied I think from the code point of view this getting ready.

From the design/UX point of view, we are not there yet, getting there would create an even bigger PR making it impossible to review in one go so we will need some follow up PR's.
For reference here are some images of the current behavior:

Apr-12-2019 12-29-48

Apr-12-2019 12-32-54
(ends with using the keyboard to navigate around all the items).

I guess there are two options:
We merge this PR with the blocks exposed as they are now (behind the phase 2 flag so they appear only on the plugin) marked as experimental.
We merge but we add an {inserter: false } flag making it impossible for anyone to notice these blocks. E.g: they don't appear anywhere unless the flag is changed.

I guess we should go with option 1 if we think these samples already provides some value and may be useful and we want testing to be easy.
We should go with option 2 if we think we are still in a very early stage and we prefer to keep the blocks hidden, making people that want to test the block need to change the flag.

Any thoughts on this @mapk, @melchoyce, @sarahmonster?

@sarahmonster

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

👍 I've gone ahead and requested some re-reviewing, and if we need extra help we can surface in this weeks' core-editor chat. Then we can move on to the next stages! Sound good?

@earnjam

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I haven't tested this out yet, but on a quick pass my main concern is this completely changes the way nav menus are stored in the database. That's going to be a concern for backward compatibility.

Currently nav menus are stored as a taxonomy term, with each item stored as a post and assigned that term. I'm not sure if those would be accessible to this block once we merge a menus endpoint (@wpscholar has put together a patch that covers all CRUD operations on menus)

Even if a nav menu block is saved as a reusable block, this PR would basically store everything for that menu as a blob inside of post_content. This makes it much more difficult to access and parse from an API perspective.

It also means we'd have to have 2 different ways to return menus via the REST API depending on how they were created.

The current storage method could still allow rich block-based menus. Since individual menu items are already stored as posts, their extra block markup could be stored in post_content. I think the main thing that would need to be figured out is how to save any settings that apply to the menu as a whole.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@earnjam I think we should distinguish the menu block from the current menus. In the block editor, blocks have their data as attributes, blocks can be used in several contexts, inside post_content, soon maybe inside widget areas, and potentially later in "global templates". Blocks can also be used in a WP agnostic context.

For this reason, I think the BC concern is outside of the scope of this PR. The block is going to be used like any other block and should work in any of these contexts. Whether this block can be used in the menu screen and its attributes mapped to the existing storage is a separate question that is not impacted by this PR.

@earnjam

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@youknowriad I understand the desire for a platform agnostic menu block, but perhaps that block shouldn't be the default menu block for WordPress.

Because that means we're either excluding traditional WordPress menus from being placed into areas that blocks can be used, or we'll require another "legacy menu" block that allows them to be inserted, which is just confusing.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I don't think we should base our future decision on what the WP menu block should be on the fact that WordPress has already existing menu concept. We should be thinking about what is the best menu block we could build for the future and then try to adapt it to work in WordPress.

Because that means we're either excluding traditional WordPress menus from being placed into areas that blocks can be used or we'll require another "legacy menu" block that allows them to be inserted, which is just confusing.

We don't need any other block, we already have the "Classic Widget" block than can be used for the existing menus.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

Why do you think so? For the future, I expect APIs to retrieve blocks composed of attributes.

@earnjam

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

We don't need any other block, we already have the "Classic Widget" block than can be used for the existing menus.

That's going to be pretty confusing for users. They want to insert a menu and choose the menu block, but then can't use the menu that they've already created. It isn't intuitive at all that the Legacy Widget block should be used for inserting menus.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

Why do you think so? For the future, I expect APIs to retrieve blocks composed of attributes.

Let's say a site has a traditional menu and a user creates a menu using this block. How would you retrieve both of those via the REST API?

The API either needs to handle the logic of retrieving and parsing two types of data, or else there are two endpoints.

@sarahmonster

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

That's going to be pretty confusing for users. They want to insert a menu and choose the menu block, but then can't use the menu that they've already created.

Agreed! This is why our original design proposal suggests the ability to, upon creating a new menu block, convert your existing menus to blocks.

Nav- Smart default decision tree

...where "User selects from existing menus" looks something like this:
Existing menu selection

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Let's say a site has a traditional menu and a user creates a menu using this block. How would you retrieve both of those via the REST API?

By asking the question, you assume "menus" are an important concept that needs to fetched using the REST API. This assumption is completely true today (if we omit some menus created in an adhoc way including plugins to create menus) but is this assumption valid for the future?

Potentially, In terms of content, there's mainly two/three concept that matters for the user:

  • CPTs (posts, pages...)
  • Blocks in the content of these CPTS
  • Templates assigned to these CPTs.

Content creation happens through these-screen (editors) and based on blocks. Menus are less important and represent just another block. Third-party blocks can create custom menu blocks, custom menu items. In fact, a "Recent Post" block is not much different from a menu, it's just a menu composed of given posts/pages... A list of buttons/links aligned next to each other in templates are also menus, what matters to the user at the point is the fact that the links work as expected and that they are global and edited in a single place.

What is a menu in that case? Can you fetch recent post menu items today? Is it important for the user at that point to fetch menu items of a given block but not the other? I think it's more important for people to fetch blocks that compose templates and blocks that compose post_content at that time.

To be honest, I think we don't know all the answers to these questions. I don't think we should assume things and adapt the block editor's framework for something that we can't know for sure.

That's why I suggest that we build the best menu block we can think of UI-wise first and iteration will inform us about the best storage system, how do users would want to consume the blocks...

@earnjam

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Thanks @youknowriad I don't want to be argumentative here. I appreciate you taking the time to outline your thoughts.

I just want to make sure to voice some backward compatibility concerns. But I guess we are taking the same sort of mindset as the editor rollout. i.e. At a certain point we replace the menu screen entirely and people can just convert existing menus to new block-based menus when they go to edit them.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Thanks @earnjam for the understanding. I definitely don't want to dismiss any feedback and BC is still an important concern. It's important that we keep iterating in the plugin which will allow us to discover the shortcomings, the BC concerns and the use-cases that we may not cover properly and we'll have to adapt as these get clarified.

@sarahmonster

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Hello everyone! @jorgefilipecosta has informed me we can merge this PR without committing to a particular direction in terms of how the menu items are stored for now, and progress on the development of this block is still stalled pending getting this merged.

Let's continue the ongoing debate, but if someone has time to review this PR and get it over the finish line, we'd really appreciate that. ❤️

@spacedmonkey

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Is it know theme creators will need to do support this new block? The css can get pretty complex for menus and menus are likely to look extremeley different depending on where there are on the site. Menus in a footer would be different from in content or sidebar for example. Is there going to be a way for developers to opt in to this block? Automatically opting them in will like end up with broken blocks on the front end.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Is it know theme creators will need to do support this new block? The css can get pretty complex for menus and menus are likely to look extremeley different depending on where there are on the site. Menus in a footer would be different from in content or sidebar for example. Is there going to be a way for developers to opt in to this block? Automatically opting them in will like end up with broken blocks on the front end.

Hi, thank you for bringing these points. Right now the plan is to have a standard block and polish its UI, making improvements in the process ( to InnerBlocks for example) that any block developer can take advantage. There are ways themes can use to disable a specific block, regarding the styles the block will provide a default set of styles like other blocks do, and themes can opt-in to remove some of this styles.

This PR was rebased taking into account updates that happen meanwhile, and the changes in #14867, it should be ready to merge after #14867.

@draganescu
Copy link
Contributor

left a comment

I have tested locally and the blocks work in this alpha state. It works as described! Also the code itself is a stub for a lot more functionality to be added so we'll see what the final form will be.

Committing this hidden away is a step forward so we can continue with smaller, more focused PRs for each part of the navigation menu block.

@jorgefilipecosta there is though a failing "Filter merged" check , not sure yet what this is about, never seen that before, so if you could let me know or fix it we can move along 💃

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from 8d3230c to 11fdc2d Jun 17, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from 11fdc2d to ab7ad0e Jun 17, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/navigation-blocks branch from ab7ad0e to da17cc9 Jun 17, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Hi @andreilupu, I rebased this PR, there are no errors happening now. Before merging this PR we need to merge #14867, very simple PR just passing down a prop.

@andreilupu

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Hi @andreilupu, I rebased this PR, there are no errors happening now. Before merging this PR we need to merge #14867, very simple PR just passing down a prop.

As much as I'm honored about the @ reference, I think that the shout out was for @draganescu

I can still test the PR later if it's needed.

@draganescu draganescu merged commit 4d6a2a5 into master Jun 18, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.0 milestone Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.