Skip to content

Conversation

@theodesp
Copy link
Member

@theodesp theodesp commented Jan 16, 2025

Summary

This PR adds support for resolving and returning navigation items within the CoreNavigation block for WPGraphQL Content Blocks. The implementation parses the referenced navigation post and returns a list of block types (EditorBlock) representing the inner blocks of the navigation.

Note

The implementation does not consider the list of allowed blocks so we do not do any validation checks as we rely on Gutenberg logic.

Usage Example

With this update, the GraphQL query below is now supported:

{
  posts {
    nodes {
      editorBlocks {
        ... on CoreNavigation {
          type
          name
          innerBlocks {
            type
            name
          }
          attributes {
            ref
          }
        }
      }
    }
  }
}

The navigationItems field will return a list of parsed blocks from the referenced navigation post, which includes nested block types like CoreNavigationLink.

Example Response

{
  "data": {
    "posts": {
      "nodes": [
        {
          "editorBlocks": [
            {
              "type": "CoreNavigation",
              "name": "core/navigation",
              "innerBlocks": [
                {
                  "type": "CorePageList",
                  "name": "core/page-list"
                },
                {
                  "type": "CoreNavigationLink",
                  "name": "core/navigation-link"
                }
              ],
              "attributes": {
                "ref": 31
              }
            },
            {},
            {},
            {}
          ]
        },
        {
          "editorBlocks": [
            {}
          ]
        }
      ]
    }
  },
}

@theodesp theodesp requested a review from a team as a code owner January 16, 2025 13:58
@changeset-bot
Copy link

changeset-bot bot commented Jan 16, 2025

🦋 Changeset detected

Latest commit: 61b1ccf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wpengine/wp-graphql-content-blocks Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@justlevine justlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal opinion here is the same as on #331 (review) - and will be any time a PR:

  • registers a custom field to a Block (maintainability, forward-compat).
  • calls render_block() (performance, bugs from rerendering)

Doubly true here, when (like with CoreList), CoreNavigationLink is already available using the proper pattern of innerBlocks / flatListToHierarchical().

image

colinmurphy
colinmurphy previously approved these changes Jan 16, 2025
@theodesp
Copy link
Member Author

theodesp commented Jan 16, 2025

My personal opinion here is the same as on #331 (review) - and will be any time a PR:

  • registers a custom field to a Block (maintainability, forward-compat).
  • calls render_block() (performance, bugs from rerendering)

Doubly true here, when (like with CoreList), CoreNavigationLink is already available using the proper pattern of innerBlocks / flatListToHierarchical().

image

TBH innerBlocks does not render anything for me:

{
  "data": {
    "posts": {
      "nodes": [
        {
          "editorBlocks": [
            {
              "innerBlocks": [],
              ...
}

{
  "data": {
    "posts": {
      "nodes": [
        {
          "editorBlocks": [
            {
              "innerBlocks": [],

IF you look at the actual resolved block data its just this. There is nothing there except the ref attribute

{
 "blockName": "core/navigation",
 "attrs": {
   "ref": 31
 },
 "innerBlocks": [],
 "innerHTML": "",
 "innerContent": [],
 "clientId": "67892bb3d0bb0"
}

So if we could expose the list of innerBlocks by just returning the parsed list of blocks from the CoreNavigation. Is there a better way you think?

@justlevine
Copy link
Contributor

justlevine commented Jan 16, 2025

So if we could expose the list of innerBlocks by just returning the parsed list of blocks from the CoreNavigation. Is there a better way you think?

If it's not working on post.editorBlocks, then my bet is it's a global state issue since the query is happening at the post-level, but navigation is meant to occur outside the post_content (probably addressed in rtCamp#31 that I havn't backported yet 😬. I can take a look over the weekend).

With that in mind, I'd even go so far as to say "let CoreNavigation be buggy for now" - there are more than enough "content" blocks that are missing parity with GB.

If you do need to handle it in the interim - my recommendation would be to populate it ContentBlocksResolver, similar to how we currently handle reusable blocks: https://github.com/wpengine/wp-graphql-content-blocks/blob/b133a1b36d9e5bfb407e46931c0c279b7a62dc43/includes/Data/ContentBlocksResolver.php#L252C1-L270C3.

Even if worst case you add another render_block() I need to strip out in a future refactor, at least the schema won't require maintainance/deprecation/eventual breaking change, and it'l still be more performant to do it with the rest of the parsing vs calling it separately on every field resolution.

Copy link
Member

@whoami-pwd whoami-pwd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor code related issues here that we can improve.

@theodesp
Copy link
Member Author

https://github.com/wpengine/wp-graphql-content-blocks/blob/b133a1b36d9e5bfb407e46931c0c279b7a62dc43/includes/Data/ContentBlocksResolver.php#L252C1-L270C3

Thanks. I will update the code then.

colinmurphy
colinmurphy previously approved these changes Jan 20, 2025
Copy link
Member

@whoami-pwd whoami-pwd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍🏻
Just a couple of minor suggestions.

whoami-pwd
whoami-pwd previously approved these changes Jan 20, 2025
@theodesp theodesp dismissed stale reviews from whoami-pwd and colinmurphy via 4f0779a January 21, 2025 10:53
@theodesp theodesp merged commit b813352 into main Jan 22, 2025
12 checks passed
@theodesp theodesp deleted the feature-core-navigation-block-resolve-blocks branch January 22, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants