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

Avoid hard coding group block name in transform #25627

Closed
wants to merge 1 commit into from

Conversation

willrowe
Copy link

@willrowe willrowe commented Sep 24, 2020

Description

This allows the block to be extended in a block with a different name. I have overridden core/group with a custom version of the group block, but it will not allow transforming since this is hard-coded.

How has this been tested?

This should be covered by existing tests.

Types of changes

This should function the same in all existing usage, but allow transforms using the name of the block it is embedded in.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

This allows the block to be extended in a block with a different name.
@talldan
Copy link
Contributor

talldan commented Sep 25, 2020

I have overridden core/group with a custom version of the group block, but it will not allow transforming since this is hard-coded.

@willrowe Thanks for creating this PR. I think this PR needs a bit more information in the description currently for this change to be considered.

Would you be able to share more details about how you've overriden the core/group block?I think it's worth exploring other possible solutions before changing the group itself.

@talldan talldan added [Block] Group Affects the Group Block [Status] Needs More Info Follow-up required in order to be actionable. labels Sep 25, 2020
@willrowe
Copy link
Author

@talldan thanks for taking a look. This does not change the functionality of the group block itself, since this.blockName will still return core/group. This change specifically allows the transform to be used in a custom "grouping" blocks. Recent changes like #15774 allows a custom block to be registered as the default "grouping" block, further supporting the ability to replace or extend the built-in core/group block. My main concern here is extensibility and not needing to duplicate code that I do not need to alter the functionality of, which in turn helps with maintainability.

@talldan
Copy link
Contributor

talldan commented Sep 25, 2020

This change specifically allows the transform to be used in a custom "grouping" blocks.

@willrowe I'm still unsure about how this can be used for a different block when it's a transform defined specifically for core/group. I think if the new custom group block that you've created needs a similar transform you could define some similar code in its block settings.

@willrowe
Copy link
Author

I'm still unsure about how this can be used for a different block when it's a transform defined specifically for core/group.

As far as I can tell, this code prevents a single block of the same type from being nested inside of it. This is not unique to just the core/group block, but to "grouping" blocks in general.

I think if the new custom group block that you've created needs a similar transform you could define some similar code in its block settings.

In my case, it's exactly the same besides the block name. That's why I'm proposing this change. It would be helpful to only need to override the parts that are different and to be able to leave any default behavior intact without needing to duplicate it.

Do you have any specific concerns about something breaking with this change? Otherwise, I understand if this isn't the direction that you want things to go as far as being able to extend core blocks.

@talldan
Copy link
Contributor

talldan commented Sep 30, 2020

I see, so you're not completely declaring a new group block, but overriding the core block and changing aspects of it, including the block name.

Generally the recommended approach is to completely write a new block with it's own unique name, which can be set as the editor's group block using the wp.blocks.setGroupingBlockName function.

Another way is to extend the existing core block without changing its name—there are quite a few plugins that do this to add features to blocks. The latter approach is more likely to experience breaking changes if any aspect of the core block change in an incompatible way (which despite the best intentions can happen depending on how the block is modified).

I realize creating an entirely new block feels like a lot of duplication of code, but it's generally more maintainable this way for both parties—your block is less likely to break from core changes, and the core code is more maintainable in terms of the group block not having unique undocumented characteristics.

See also this talk/article for some other perspectives: https://www.deconstructconf.com/2019/dan-abramov-the-wet-codebase.

I'll close the PR, but hopefully the discussion gives you some paths forwards.

@talldan talldan closed this Sep 30, 2020
@willrowe willrowe deleted the patch-2 branch September 30, 2020 12:34
@willrowe
Copy link
Author

Another way is to extend the existing core block without changing its name

Ideally this is how I would prefer to do it, but I cannot on the project I am currently working on since the name has already been changed. When I was going through the code I just thought it was odd that the block name was hardcoded instead of using the block name on the bound object that is available dynamically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block [Status] Needs More Info Follow-up required in order to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants