Make WordPress Core

Opened 16 months ago

Closed 12 months ago

Last modified 12 months 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: 6.5
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 16 months ago.
Add a new renderer class.
patch.2.diff (20.6 KB) - added by scruffian 16 months ago.
Adding tests

Download all attachments as: .zip

Change History (10)

@scruffian
16 months ago

Add a new renderer class.

@scruffian
16 months ago

Adding tests

#2 @scruffian
16 months ago

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

#3 @isabel_brison
15 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
15 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
12 months ago

  • Milestone changed from 6.5 to Future Release

#6 @youknowriad
12 months 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
12 months ago

  • Milestone set to 6.5

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


12 months ago

Note: See TracTickets for help on using tickets.