Make WordPress Core

Opened 4 months ago

Closed 11 days ago

Last modified 11 days ago

#59867 closed enhancement (worksforme)

Navigation Block Rendering: Move code to a class

Reported by: scruffian's profile scruffian Owned by:
Milestone: 6.5 Priority: normal
Severity: normal Version: trunk
Component: Editor Keywords: gutenberg-merge has-patch
Focuses: Cc:

Description

The navigation block render function is very long and hard to reason about. In Gutenberg we have moved the code to a class, and split it up into smaller functions so its easier to reason about and change.

I am opening this patch now to get this change made in core in advance of WordPress 6.5 to give us plenty of time to review it.

I didn't migrate the tests as we don't seem to have tests for blocks in core, but I'm happy to add it if that's helpful.

Attachments (2)

patch.diff (44.4 KB) - added by scruffian 4 months ago.
Add a new renderer class.
patch.2.diff (20.6 KB) - added by scruffian 4 months ago.
Adding tests

Download all attachments as: .zip

Change History (9)

@scruffian
4 months ago

Add a new renderer class.

@scruffian
4 months ago

Adding tests

#2 @scruffian
4 months ago

I added the tests to tests/phpunit/tests/editor/class-wp-navigation-block-renderer-test.php

#3 @isabel_brison
3 months ago

  • Keywords gutenberg-merge added
  • Milestone changed from Awaiting Review to 6.5

Thanks for opening this @scruffian !

I'm afraid the patch won't work as it currently stands. Files inside src/wp-includes/blocks/ get auto-generated from the block-library package, so this change would be discarded as soon as the build ran. Given the change is already in Gutenberg, once the npm packages are published they can be updated in core.

One thing that would make this easier to review would be to convert the patch to a PR to https://github.com/WordPress/wordpress-develop (and that way we also get the tests to run on CI)

My main question regarding the overall change is whether, given this class is tied to a block-library block, it would be better for it to live in block-library and go through the same auto-generation process when added to core. Having it outside, once the class is in core the Navigation block code will be split between two codebases which will make it harder to iterate on.

Would be great to hear some more opinions on this! Cc @talldanwp @youknowriad @gziolo

#4 @youknowriad
3 months ago

I think both options can work but yeah, since it's a class that is specific to a given block, I'd leave on the same package (maybe even the same file) to continue benefiting from the automatic back-porting to Core.

#5 @swissspidy
11 days ago

  • Milestone changed from 6.5 to Future Release

#6 @youknowriad
11 days ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

I think this has been fixed actually. It's just that the class lives on the same file, so it has been backported automatically with the package update.

#7 @youknowriad
11 days ago

  • Milestone set to 6.5
Note: See TracTickets for help on using tickets.