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

Duotone: Output duotone presets per block #48374

Closed
wants to merge 13 commits into from

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Feb 23, 2023

What?

This is another angle on #48291.

If we remove the code that outputs the presets, how can we still use presets? This PR suggests saving the presets to an array and then outputting them in the footer. We do this for both block supports, and filters that are defined in the theme.json.

Why?

Outputting unused presets creates extra output that we don't need.

How?

  1. Create a new array called $duotone_presets.
  2. When we render a block we check if the block has a duotone filter applied - if it does we save it to the new array.
  3. We also check if the block has any filter settings in the theme.json - if it does then we also save this setting to the new array.
  4. We then run an action on wp_footer which outputs all the used preset duotones.

Testing Instructions

  1. Switch to a theme that has a duotone setting in its theme.json. (eg. Skatepark)
  2. Check that the images all have a duotone applied in the front end.
  3. Add an image to a post and select a preset duotone filter that is different to the main one for the theme.
  4. Check that this image also has a preset duotone filter applied in the frontend.
  5. Check that only the used SVGs are output in the frontend

Notes

Still to do:

  • Find a way to cache the theme json settings so we dont recalculate on every render
  • Combine the two wp_footer actions into one but saving custom duotones in the WP_Duotone class.

@scruffian scruffian self-assigned this Feb 23, 2023
@github-actions
Copy link

github-actions bot commented Feb 23, 2023

Flaky tests detected in 50a9763fe0b5a7ec1bea88dd3950cd2d6c6c3dd8.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4284866549
📝 Reported issues:

*/
printf(
'<script>( function() { var el = document.querySelector( %s ); var display = el.style.display; el.style.display = "none"; el.offsetHeight; el.style.display = display; } )();</script>',
wp_json_encode( $selector )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$selector isn't defined in this function. Not sure what it's needed for, but it appears to already run through the footer action above even if it's a global style, so maybe this Safari stuff isn't needed here?

@jeryj
Copy link
Contributor

jeryj commented Feb 23, 2023

I've tested this on TT3 with the Sherbert theme on a single post template with these images:

  • Featured image with duotone from global styles
  • gutenberg default duotone
  • custom selected duotone
  • Theme default duotone
  • Plain image
  • Unset duotone image

It's working in all these cases and only the duotones used on that post are being output in the <svg> footer definitions.

@scruffian
Copy link
Contributor Author

It's working in all these cases and only the duotones used on that post are being output in the footer definitions.

I think there must be something wrong with your testing setup - it shouldn't be working for all these cases yet!

}
);

function gutenberg_save_duotone_preset_svgs( $block_content, $block ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs for every block we render. That might be inefficient...

$styles_variables = '';
if ( in_array( 'variables', $types, true ) ) {

// We need to do this first for duotone.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elucidate this!

@jeryj
Copy link
Contributor

jeryj commented Feb 24, 2023

@scruffian - I branched off of this since I was doing one gigantic refactor and couldn't split it into smaller commits. It is working though (I think)! 🎉 #48436

If you think it's a good approach, feel free to merge it into this branch and keep going with it.

scruffian and others added 13 commits February 27, 2023 15:20
'unset' is a valid attribute to pass, but we don't want to output any svg definition for it. Checking for unset makes sure we're dealing with a valid slug that should output a corresponding svg definition.
…red blocks

I'm sorry this is in one giant commit. The general idea is:
- On wp_loaded
  - save all duotone presets defined in merged global styles
  - save all block names and their corresponding duotone slug info
- on block render, check if the block:
  - is one of the blocks using duotone via global styles
  - has any defined duotone styles
  - if so, add it to the duotone_output array to have its duotone info output
@jeryj
Copy link
Contributor

jeryj commented Mar 10, 2023

Duotone received a large refactor and has changed too much. Closing in favor of #48995

@jeryj jeryj closed this Mar 10, 2023
@scruffian scruffian deleted the update/output-duotone-presets-per-block branch March 10, 2023 22:03
@scruffian
Copy link
Contributor Author

Closing in favor of #48374

Do you mean #48995?

@jeryj
Copy link
Contributor

jeryj commented Mar 10, 2023

haha - yes! Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants