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

Discussion: Proposed ACF Block Changes in 5.13 (or later) #569

Closed
lgladdy opened this issue Nov 11, 2021 · 22 comments
Closed

Discussion: Proposed ACF Block Changes in 5.13 (or later) #569

lgladdy opened this issue Nov 11, 2021 · 22 comments
Assignees

Comments

@lgladdy
Copy link
Member

lgladdy commented Nov 11, 2021

In ACF 5.10 we moved to version 2 of the WordPress Blocks API for WordPress 5.6+ which supports more advanced functionality such as extraProps support. Unfortunately, our implementation of this triggered re-rendering of the blocks anytime a property changed as we have to store those values in properties of the block rather than directly inside the markup the block generated. This caused issues with other block editor features such as block styles and the built in spacing/padding support. ACF 5.11 removes this support of extraProps to prevent these re-renders which will improve performance and solve issues with block styles etc.

These re-renders happen because we translated WordPress's computed styles and classes into properties on the block - because ACF blocks contain no markup in the block editor to apply those computed attributes to, unlike any other block.

In an upcoming release (likely 5.12), we plan to re-introduce this support, but in order to do so will change the structure of ACF blocks to include a HTML wrapper outside of your template which will contain any extra props, spacing and custom styles. This is a potentially breaking change, depending on how specific your CSS styles are at targeting your template elements inside the block editor or front end output, or if you’re already translating the internal block editor style properties into valid CSS for margin and spacing in your template.

Currently, when you set spacing or padding, you’re passed through internal block editor markup which is not valid CSS to your template which we know some users are using to build valid CSS from to apply padding/spacing to their blocks. As part of the new wrapper we intend to introduce, this markup will automatically be applied to the wrapper element as valid HTML, so any spacing, margin or custom styles added in the block editor UI will automatically be applied inline - just like any other block.

We want to ACF blocks to work as similarly to default WordPress blocks or native blocks as possible, so believe this change is needed as default, but would like to gauge your opinion here on GitHub as to if we should support an opt-out for this behaviour when you register a block - at the expense of supporting things like extraProps and any future WordPress block editor features that modify the block properties, or plugins that do similar.

This thread has been created to gather feedback from users on how you use CSS targeting already, and how you'd like to see us implement this change.

@lgladdy lgladdy self-assigned this Nov 11, 2021
@Graphnic
Copy link

Hello, thanks for all your work on ACF since you took over.

Totally get why you are proposing this.

We have been using CSS logic to automatically determine block spacing based on the class set on the previous block. For example, if two adjacent block have the same background colour set, we'll reset the top padding of the second block so as to maintain a visually consistent space between them, but if they have different colours we'll keep the top padding. It's pretty basic CSS declarations that look for classes on our outer wrapper, which are in turn determined by ACF fields on the block settings.

For a simplified example, in order to remove the top padding on 'block 2' (because it has the same background colour as block 1) we might use something like:

<div class="my-block-wrapper bg-blue">Block 1 content</div>
<div class="my-block-wrapper bg-blue">Block 2 content</div>
<div class="my-block-wrapper bg-green">Block 3 content</div>
.my-block-wrapper  { padding: 100px 0; }
.bg-blue { background: blue; }
.bg-blue { background: green; }
.my-block-wrapper.bg-blue + .my-block-wrapper.bg-blue { padding-top: 0px; }

A new 'outer wrapper' on the frontend would of course break this logic, and I don't think we can adapt the CSS to work around it.

I fully acknowledge our approach is probably sub-optimal - and a bit old-school, but it works really well and I know others are using a similar technique also. I'm very open to doing this a better way, but I'd like to not have to revisit live sites rework the codebase, so an opt-out would be really helpful.

Gutenberg has been a mightly uphill struggle for some of us and ACF has helped that journey immensely, so thanks. I suspect the solution here is for us to get deeper into doing things the 'WordPress way'... it's just that the 'WordPress way' seems to be constantly changing at the moment and keeping up is a challenge as a small multidisciplinary studio with limited resources!

@lgladdy
Copy link
Member Author

lgladdy commented Nov 11, 2021

Hey @Graphnic - Cheers for your input here. it's really useful to see real world examples, I certainly don't think you're alone in this kind of specific workarounds to get things working as you'd expect.

You're right that the "WordPress" way here would probably be to use some of the new layout options like columns or rows with custom classes - or Block Styles - but things have moved so fast in Gutenberg, and continue to do so, so we fully understand that virtually everyone is doing things in different ways here.

If we supported something like a "v2: false" switch in acf_register_block_type supports array, where we keep the behaviour in 5.11 for your site, would that be enough? I appreciate you'd need to still deploy each of your old sites with this update before we release 5.12, so keen to understand if you think this would be okay? Obviously we'll agree on that schema name early and publish it so users can have several weeks to roll out that change before we release 5.12.

@Graphnic
Copy link

Yes, for me that would be perfectly fine to maintain compatibility. That change is super easy to deploy.

What it doesn't answer - and I appreciate it's not your problem per-se - is how we would replicate our styling functionality for sites currently in development? There have been so many changes in the block editor, some of which breaking, that this felt like a much safer approach than adopting bleeding-edge features only to have to spend a stack of support time fixing things later on when things change!

Block styles do probably present a partial solution, which we have used to an extent. I don't think they are as flexible as the logic we can construct with the combination of ACF fields and PHP defining exactly what classes are applied to the block wrapper though?

To cut the question a different way, would there be another way we could incorporate logic based on field values to define which classes are added to your new outer wrapper?

@lgladdy
Copy link
Member Author

lgladdy commented Nov 15, 2021

@Graphnic It's certainly an interesting use-case, but I don't think it's one that's currently "supported" by the block editor and blocks. Blocks are designed to be standalone things without any interactions to the blocks around it, outside of them being in a layout outer block.

That said, your specific question of how to use the value of fields to set outer block contains does feel like it might be possible using extraProps. It would require a whole bunch of custom code there, as you only get properties of the block so would have to do AJAX calls inside to get data about the block... alternatively, you could do use JavaScript to add classes inside your template - that's probably the easiest way.

There's also things like block patterns which are likely to become a bigger deal in 5.9 with full site editing. In that case, you can define a combination of ACF blocks with specific classes on that outer block already once we support the outer wrapper.

@Graphnic
Copy link

Graphnic commented Nov 15, 2021

@lgladdy in our use case, assuming you will be rendering $block['backgroundColor'] on the new wrapper (which is the plan, right?), I think we can cover 90% of our usual spacing logic by applying them to the colour names we specify in editor-color-palette, rather than basing them on custom fields. We are perhaps trying to be too clever.

If you're not planning to render $block['backgroundColor'] on the wrapper, could it be considered as an option for a control on the Field Group settings for the block?

@lgladdy
Copy link
Member Author

lgladdy commented Nov 15, 2021

@Graphnic Indeed, for backgroundColor that should be how it works. We leave the decisions on what styles and classes get applied to WordPress, and the filter-able chain that turns everything from the properties panel into actual markup. We're also interested to see what changes arrive in tomorrow's WordPress 5.9 Beta 1 as the changes for Full Site Editing are likely to have some impact here too.

@Graphnic
Copy link

@lgladdy 'We leave the decisions on what styles and classes get applied to WordPress' is always a bit of a worry🤣. I've taken some comfort in the fact that regardless of what Core does, my ACF block templates will stay intact as they are driven by my own markup and classes, and not WordPress's. That's perhaps not a helpful mindset though.

@lgladdy
Copy link
Member Author

lgladdy commented Nov 15, 2021

@Graphnic You're right - my wording there wasn't ideal! I just meant we're intending on using the WordPress "default" system to handle that for us, and expect it to work like it does now! We'll obviously have way more info on all this during the development cycle, and will make sure we fill any gaps as required. We're wanting to gather feedback as soon as possible to see if any users raised issues or ways they were using blocks that we needed to take into account before we progressed that route.

At the same time though, we're hoping to gain support for future WordPress core things for "free" by using this method too, as has been the case in things we've not been able to support in recent releases because we didn't support blocks v2.

@thomasmb
Copy link

thomasmb commented Nov 15, 2021

In the way we work today, we add the outer wrapper ourselves like this:

<div <?php Utilities\block_attrs( $block ); ?>>

In that function we add the ID, classes (block name class, colors, etc) and everything else that needs to be added. Here is a short version to show what it currently does:

function block_attrs( $block, $classes = [], $atts = [] ){

	// in case classes are passed as string
	if( !is_array( $classes ) ) {
		$classes = explode( " ", $classes );
	}

	$classes[] = str_replace( "acf/", "block-", $block['name'] );

	// add align class if set
	if( !empty( $block['align'] ) ) {
		$classes[] = 'align' . $block['align'];
	}

	// Same for added classes, colors, background and all the other supported features
    
	// Loop over attributes and output them
}

Now if ACF brings this change, with and opt-out, we would have to update tons of projects to handle the new method (even just adding an opt-out to a lot of block). I'm sure we wouldn't be the only ones.

Would it be an option to instead provide a similar function? The function itself would sort of be an opt-in, and it wouldn't break any current implementations. It could also allow for passing any additional classes like shown in our function (or post_class) or even attributes.

@lgladdy
Copy link
Member Author

lgladdy commented Nov 16, 2021

@thomasmb We need to let WordPress and the block editor handle all this in JS on the front end unfortunately - otherwise we lose support for filters and potential future things introduced by WordPress Core.

Our solution we shipped in 5.10 but had to roll back worked a little bit like you suggested - we took the markup generated by WordPress and added it to attributes of the block comment so we could let users handle this in their templates - but each time we updated those attributes, it triggered a re-render outside of our control.

We're not hard set on this change being opt-out yet though.. and the more we think about it, the more we think we might have to make it opt-in (my latest thinking is block validation due to the new wrapper might make all blocks created previously show as errored, so we'd have to make it opt-in in that case!)

Either way, I think we'll take a stab and get a build created to let users trial this and give feedback based on working code, rather than theory. Give us a couple of weeks!

@Graphnic
Copy link

@lgladdy Could you provide a mechanism in the Field Group settings to add additional classes to the block wrapper? Although not conditional/programatic, this would at least give folks who want add additional styling classes to the outer wrapper a simple way of doing it when they no longer have access to it via the template.

@lgladdy
Copy link
Member Author

lgladdy commented Nov 16, 2021

@Graphnic that has potential - but it would have to be through the block definition code I think? (acf_register_block_type) (although that said, I'd love to also bring support for the block json definitions for ACF blocks!)

@Graphnic
Copy link

@lgladdy Sure, via the block definition code would be fine as a quick win to mitigate some of the potential issues for folks. Supporting block.json should be on the roadmap if the aim is to be as native as possible. I'm only just getting into theme.json though!

@brigdev
Copy link

brigdev commented Dec 2, 2021

How would this proposed change work in a strongly css grid-based theme?

@Graphnic
Copy link

Graphnic commented Dec 2, 2021

@lgladdy I have just come up against another scenario which may be somewhat impacted by this. We have some blocks that are only rendered for users with specific user roles in WordPress, again determined by an ACF field. While that logic could still exist inside the wrapper, it would result in an empty DIV, potentially with padding applied.

I don't think there is a native way of handling role based permissions at a block level, hence we've added controls with ACF. I know it's a bit of an edge-case.

@lgladdy
Copy link
Member Author

lgladdy commented Dec 2, 2021

@Graphnic Thanks - we'll think about that and figure out a way to make that viable...

For what it's worth, this change will (almost certainly) come after ACF 5.12 (We're going to focus on getting everything with blocks working in Full Site Editing and the new Widget editor in WP 5.9) - and will definitely be opt-in by a block configuration variable.

@brigdev I'm not sure this would have too much impact - as it would only affect new site builds (or when you opt in) so you could apply the grid to where you need it too based the WordPress classes for the blocks that get applied.

@lgladdy lgladdy changed the title Discussion: Proposed ACF Block Changes in ACF 5.12 Discussion: Proposed ACF Block Changes in 5.12 (or later) Dec 2, 2021
@brigdev
Copy link

brigdev commented Dec 2, 2021

@lgladdy Thank you. My concern is for a site that is being built, due for release early January. I'm concerned about having to "opt out" of future functionality for such a fresh build.

@metropoliscreative
Copy link

@thomasmb's suggestion of a helper function that behaves similar to useBlockProps in the Gutenberg API would be the most intuitive. This way developers could choose to easily add a wrapper in their templates on a block by block basis, or choose to manually add specific block props in specific places as needed.

At the very least, we need control over the HTML element used for the wrapper, if it's applied automatically. This will be particularly important on nested blocks.

In order to align with the development approach of native WP blocks, developers should have full control over the rendered markup.

@lgladdy
Copy link
Member Author

lgladdy commented Dec 20, 2021

@metropoliscreative Yeh, I fully agree on making sure devs have control of the markup. One of the ideas I've been pondering is if we can do this by letting WordPress apply the styles to a container html element that we remove before passing to the template. That way, WordPress's react layer can still do everything it wants now and in the future, and we can parse out classes etc from that to pass into the template to let users do what they want.

I think this would still need to be opt in because it would change the markup stored inside the block, and I think WordPress would see that as a validation error for blocks added before the change and error them all immediately upon updating - but we'd need to test that fully once we've written that logic.

This plan is also a little more complicated by the way InnerBlocks work at the moment - we currently use any content inside the block comment as InnerBlocks - so we'll need to parse that out too.

@lgladdy lgladdy changed the title Discussion: Proposed ACF Block Changes in 5.12 (or later) Discussion: Proposed ACF Block Changes in 5.13 (or later) Feb 2, 2022
@coreyworrell
Copy link

Just wanted to include mention of this, as it is very useful for server rendered blocks: get_block_wrapper_attributes()

@lgladdy
Copy link
Member Author

lgladdy commented May 12, 2022

Hey everyone, today we released a preview build of our next release of ACF which contains many of the changes we discussed here.

You can find more information here: #654

Please join us in testing and let us know your feedback over there!

@lgladdy
Copy link
Member Author

lgladdy commented Jun 10, 2022

Hey everyone!

6.0.0-alpha2 was made available yesterday which contains the markup changes we've discussed here. Please test it and give us feedback over on #654! We'll close this issue now and pick up any feedback over there.

Thanks!

@lgladdy lgladdy closed this as completed Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants