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

Make sure theme color palette presets are output when appearance tools are enabled. #57190

Merged
merged 3 commits into from Dec 20, 2023

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Dec 19, 2023

What?

Partially addresses #56131, namely

Border with preset color does not show correctly on site front end due to preset CSS for border not being output

When outputting preset styles based on color palettes, only the default palette is used unless the theme supports theme.json.

But it is possible for classic themes to define color palettes as a theme support, and it is also possible for themes to add support for appearance tools, or just for border, which enables design tools such as border that will leverage the theme color palette for their presets. This means that, if both color palette and either appearance tools or border are supported by the theme, theme preset styles should be output in addition to defaults.

Note: currently appearance tools support in themes is limited to Gutenberg. There was an attempt to add this feature to core, which was reverted because not all tools worked well with classic themes. This PR is a step towards addressing these issues so that support for appearance tools can be re-enabled in core.

Testing Instructions

  1. Add add_theme_support( 'appearance-tools' ) to the setup function of a classic theme.
  2. In the block editor, check that core blocks with border support now show the border controls.
  3. Try adding borders to several blocks, e.g. Group, Image, Details. Try with preset and custom colors, and also changing width, style, radius where possible.
  4. Save and compare the editor with the front end: everything should match.
  5. Check that the correct styles are output for preset border colors in the front end.
  6. Repeat the steps above with add_theme_support( 'border' ) instead of appearance-tools.

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2023-12-19 at 4 33 18 pm

@tellthemachines tellthemachines self-assigned this Dec 19, 2023
@tellthemachines tellthemachines added [Type] Bug An existing feature does not function as intended Needs PHP backport Needs PHP backport to Core [Type] Enhancement A suggestion for improvement. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi and removed [Type] Bug An existing feature does not function as intended labels Dec 19, 2023
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/global-styles-and-settings.php

@ramonjd
Copy link
Member

ramonjd commented Dec 19, 2023

Nice work tracking this bug down!

To test I added new Group/Code/Image/Cover/Details/Columns/Search blocks all with a mixture of border styles inc. preset and custom colors.

On trunk, where a preset color has been used, the color will not be output on the frontend.

This PR fixes that 🎉

Trunk This PR
Screenshot 2023-12-19 at 4 48 21 pm Screenshot 2023-12-19 at 4 48 33 pm

I also checked:

✅ No other preset styles, such as text/background, are affected
✅ No regressions in block or hybrid themes
✅ I can't see any issue with other styles, e.g., typography, color, background. All are applied on the frontend as before.

If possible, adding unit test coverage for this test case (core tests here for reference) would be great to guard against any regressions.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Nice work tracking this bug down!

+1, I like the way the condition is guarded, too, it means we're really only outputting these additional styles when we know that the theme wants them 👍

This is testing nicely with a variety of Classic themes with custom and preset colors, and I didn't run into any regressions:

TwentyTwenty TwentyTwentyOne
image image

LGTM! ✨

@tellthemachines
Copy link
Contributor Author

I just realised that as of 6.3 there's a dedicated border support that we should probably consider here. From @carolinan's testing instructions it seems that the issue this PR fixes was known at the time but wasn't considered to be a bug.

@andrewserong
Copy link
Contributor

Oh, good find! In the case of appearance-tools since it also opts-in to borders, would the fix here still be appropriate to land?

@tellthemachines
Copy link
Contributor Author

would the fix here still be appropriate to land?

Yes, because if we only check for border support it won't work for a theme that enables appearance-tools. I think we need to check for either support in the condition.

@andrewserong
Copy link
Contributor

I think we need to check for either support in the condition.

Sounds good to me 👍

@tellthemachines
Copy link
Contributor Author

tellthemachines commented Dec 20, 2023

If possible, adding unit test coverage for this test case (core tests here for reference) would be great to guard against any regressions.

@ramonjd do you mean adding coverage for this change to the core unit tests as part of the sync PR? I notice that there isn't a unit test for this function in Gutenberg, probably to avoid duplication?

@tellthemachines
Copy link
Contributor Author

OK I've updated this to address border support as well as appearance-tools, and edited the PR description accordingly.

Copy link

github-actions bot commented Dec 20, 2023

Flaky tests detected in da5ad04.
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/7269288876
📝 Reported issues:

@ramonjd
Copy link
Member

ramonjd commented Dec 20, 2023

do you mean adding coverage for this change to the core unit tests as part of the sync PR? I notice that there isn't a unit test for this function in Gutenberg, probably to avoid duplication?

Either / Or. If it's easier to do it in the Core patch, which it probably is, then 👍🏻

Otherwise we'd have to scaffold the test suite again in Gutenberg just for a single assertion 😄

So LGTM as is 🚢

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Just re-rested with border and appearance-tools separately, and this is still testing nicely! 🎉

@tellthemachines tellthemachines merged commit c86c37d into trunk Dec 20, 2023
51 checks passed
@tellthemachines tellthemachines deleted the fix/classic-theme-border-support branch December 20, 2023 02:35
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 20, 2023
artemiomorales pushed a commit that referenced this pull request Jan 4, 2024
…s are enabled. (#57190)

* Make sure theme color palette preset styles are output.

* Check for color palette support

* Also check for border support.
@tellthemachines tellthemachines removed the Needs PHP backport Needs PHP backport to Core label Jan 18, 2024
@getdave

This comment was marked as resolved.

@getdave getdave added the Backported to WP Core Pull request that has been successfully merged into WP Core label Jan 23, 2024
@getdave
Copy link
Contributor

getdave commented Jan 23, 2024

✅ I updated this PR with the Backported to Core label to indicate that the backport has successfully merged into WP Core. I also updated the PHP Sync Tracking Issue for WP 6.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants