#59867 closed enhancement (worksforme)
Navigation Block Rendering: Move code to a class
Reported by: |
|
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)
Change History (10)
#1
@
16 months ago
The corresponding Gutenberg PR is here: https://github.com/WordPress/gutenberg/pull/55605/files
#2
@
16 months ago
I added the tests to tests/phpunit/tests/editor/class-wp-navigation-block-renderer-test.php
#3
@
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
@
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.
#6
@
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.
Add a new renderer class.