-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Refactor server-side block support flags support #24351
Conversation
Size Change: -17 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
🤔 if this code is responsible for converting block attributes into the corresponding classes & inline styles for dynamic blocks, shouldn't this be part of the block API in the server? In other words, why would we want this code separate from the WP_Block class? |
These are seen as "extensions", We have the same thing on frontend, we don't bundle this code is block registration. |
I like that core absorbs this functionality -as we're doing in the client- so the block authors don't have to implement it for every block. I also like that we're thinking on how to extend the code. I guess what I'm missing is why this specific bit of code needs to be extension: is there any case in which we don't want to attach classes/styles to dynamic blocks? |
Right now in this PR we have three files: align.php, taxonomy.php and colors.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to refactor this, it does look much better and will definitely be more easily scalable this way.
Im still finding a couple issues and have some minor questions
// Ideally we need a hook to extend the block registration | ||
// instead of mutating the block type. | ||
foreach ( $registered_block_types as $block_type ) { | ||
gutenberg_register_alignment_support( $block_type ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be doing the same for colors and typography to initialize named and the style
attribute? While only align
seemed to be causing issues with the uses of ServerSideRender component, it would seem to be thorough to register the attributes used in all the hooks similarly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should, I just left it for a separate PR.
Ouch, what a misunderstanding 😅 Let me clarify. I definitely think that file organization is nice. My question was about why do we hook this code to the Note that I'm not familiar with this part of the codebase so may be missing some context, hence why I ask. |
Well I don't have a strong preference but we're on the plugin so we can't touch that code and second because I feel it's nicer to write things as extensions/plugins in Core, it proves our extensions API, the same question can be asked about the JS filters, why not embed the behavior in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and tests well for me. There is one more spot we probably want to use gutenberg_experimental_get
.
It's a bit hard to tell where exactly things have been changed since we moved files as well, but I think you highlighted most of the changes and after reading through things again everything appears to be good as far as I can tell (plus it all tests well ✅ ).
Thanks again for cleaning this up and improving it a bit!
The failures in the tests here are due to npm being down at the moment. |
This PR refactors the code to add block support flags support in the server for dynamic blocks.
It also allows support flags to register extra attributes automatically (done for the align one for the moment).
Testing instructions