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

Blockbase: Remove inherited style variations from child themes #6996

Merged
merged 6 commits into from
Apr 13, 2023

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Apr 6, 2023

Changes proposed in this Pull Request:

As of Gutenberg 15.1/WordPress 6.2, child themes inherit the style variations of the parent theme.

This is causing issues to some Blockbase child themes like Jackson or Kingsley, because the headstart content for a child theme is only created for the default style variation. When a different style variation is applied, the styles from the headstart content are incompatible with the new variation:

Screen.Recording.2023-04-06.at.15.51.17.mov

To mitigate that, this PR introduces a new optOutOfParentStyleVariations setting to the Global Styles, so child themes can opt out of inheriting style variations from the parent theme. The net setting has a false value by default and has only been enabled on the Jackson and Kingsley themes.

Testing instructions:

  • Apply these changes:
    • To a local/self-hosted site
    • To your WP.com sandbox (npm run deploy:push:changes and sandbox the API)
    • To a WoA dev site (p9o2xV-1r2-p2)
  • Go to Appearance > Themes
  • Activate the Jackson or the Kingsley theme
  • Go to Appearance > Editor
  • Open the Styles sidebar
  • Make sure there are no style variations
  • Go to Appearance > Themes
  • Activate any other Blockbase child theme (e.g. Attar)
  • Go to Appearance > Editor
  • Open the Styles sidebar
  • Make sure style variations are available

You can also follow instructions from D107265-code to ensure that style variations are not displayed in the WP.com design picker nor in the WP.com theme showcase.

Related issue(s):

#6941

@mmtr mmtr requested review from a team April 6, 2023 14:24
@mmtr mmtr self-assigned this Apr 6, 2023
@pbking
Copy link
Contributor

pbking commented Apr 7, 2023

Rather than the added complexity perhaps we should just consider removing the style variations from Blockbase. As a parent theme I'm rather keen to just remove it from general use anyway. I don't expect we'll be making many (any) more parent block themes either but if we do they shouldn't have their own style variations.

@mmtr
Copy link
Member Author

mmtr commented Apr 7, 2023

consider removing the style variations from Blockbase.

Sure, I'd be more than happy to do that as long as the Theme team doesn't identify any problem with that approach. It'll make things much simpler technically (it'd also remove the need of D107265-code), but then sites with the Blockbase theme activated (I believe Atomic and self-hosted sites have the ability to activate it) will lose access to those style variations. Is that fine?

@mmtr
Copy link
Member Author

mmtr commented Apr 10, 2023

Opened #7001 as an alternative approach that directly removes the style variations from the Blockbase theme.

@matiasbenedetto
Copy link
Member

matiasbenedetto commented Apr 10, 2023

Hi 👋 ! I did some testing with some Blockbase children such as Quadrat and Jackson and they seem to be working fine with the variations.

Jackson:

2023-04-10.16-59-12.mp4

Quadrat:

2023-04-10.16-54-13.mp4

As per the video of the PR description, it seems like what is looking odd are some colors of the elements used and that seems to be caused by the elements used on that post instead of a theme problem.

I'm not completely sure if that markup is coming from this pattern but they look pretty similar. Is it coming from there? I don't see that layout (Patricia Jackson, etc.) in the Jackson theme source code, where it is coming from?

If it's the same content I think the problem is related to the cover block used in the pattern. It has an overlay color set. When you change the palette of the site it may look odd. Apart from that, it doesn't make sense to use a cover to create that layout because it is not using a background image/media. The image (the circle with the line) is added as an image block.

For these reasons I think we should let the Blockbase variations as they are.
Probably we should try to solve this problem by fixing the markup used in the video of the PR description and, if we are still updating blank-canvas theme, update the pattern.

@mmtr
Copy link
Member Author

mmtr commented Apr 11, 2023

As per the video of the PR description, it seems like what is looking odd are some colors of the elements used and that seems to be caused by the elements used on that post instead of a theme problem.

Yeah sorry, I didn't a good job explaining the underlying issue in the PR description. The culprit is actually the headstart content, as I try to better explain in this other alternative PR:

This is causing issues to Blockbase child themes such as Jackson in WordPress.com, because the headstart content for a child theme is only created for the default style variation. When a different style variation is applied, the styles from the headstart content are incompatible with the new variation


solve this problem by fixing the markup used in the video

Would this work retroactively for all sites that already have the affected content generated with headstart?

@mmtr
Copy link
Member Author

mmtr commented Apr 11, 2023

Is it coming from there? I don't see that layout (Patricia Jackson, etc.) in the Jackson theme source code, where it is coming from?

It comes from wpcom-pub-themes/jackson/inc/headstart/en.json#31.

Seems to be a core/group block that has a gradient style:

<!-- wp:group {\"align\":\"full\",\"style\":{\"spacing\":{\"padding\":{\"top\":\"120px\",\"right\":\"0px\",\"bottom\":\"120px\",\"left\":\"0px\"}},\"color\":{\"gradient\":\"linear-gradient(180deg,rgb(244,253,255) 0%,rgb(255,255,255) 100%)\"}},\"layout\":{\"inherit\":true}} -->\n<div class=\"wp-block-group alignfull has-background\" style=\"background:linear-gradient(180deg,rgb(244,253,255) 0%,rgb(255,255,255) 100%);padding-top:120px;padding-right:0;padding-bottom:120px;padding-left:0;\"><!-- wp:image {\"align\":\"left\",\"id\":16,\"width\":152,\"height\":152,\"sizeSlug\":\"large\",\"linkDestination\":\"none\"} -->\n<div class=\"wp-block-image\"><figure class=\"alignleft size-large is-resized\"><img src=\"https:\/\/jacksondemo.files.wordpress.com\/2021\/12\/pattern-links-logo.png?w=304\" alt=\"A logo of a circle with a line through it.\" class=\"wp-image-16\" width=\"152\" height=\"152\" \/><\/figure><\/div>\n<!-- \/wp:image -->\n\n<!-- wp:spacer {\"height\":16} -->\n<div style=\"height:16px;\" aria-hidden=\"true\" class=\"wp-block-spacer\"><\/div>\n<!-- \/wp:spacer -->\n\n<!-- wp:heading {\"textAlign\":\"left\",\"level\":1} -->\n<h1 class=\"has-text-align-left\" id=\"patricia-jackson\">Patricia Jackson<\/h1>\n<!-- \/wp:heading -->\n\n<!-- wp:paragraph -->\n<p>Published work and ephemera.<\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:spacer {\"height\":60} -->\n<div style=\"height:60px;\" aria-hidden=\"true\" class=\"wp-block-spacer\"><\/div>\n<!-- \/wp:spacer -->\n\n<!-- wp:paragraph -->\n<p><a href=\"#\">\u201cThe Lost Tricycle\u201d Book<\/a><\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p><a href=\"#\">\"Why we must own our history\"<\/a> in The Atlantic<\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p><a href=\"#\">\"Identity and Ownership\"<\/a> in The New York Times<\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p><a href=\"#\">Sponsor: Crafty Cookies<\/a><\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p><a href=\"#\">Donate to help keep us posting!<\/a><\/p>\n<!-- \/wp:paragraph --><\/div>\n<!-- \/wp:group -->

@mmtr
Copy link
Member Author

mmtr commented Apr 11, 2023

I reviewed all the Blockbase child themes and seems that only Jackson and Kingsley present major incompatibilities between the headstart content and the style variations:

  • Jackson
Screen.Recording.2023-04-11.at.13.33.13.mov

  • Kingsley
Screen.Recording.2023-04-11.at.13.32.51.mov

There are a few other child themes that have minor incompatibilities:
  • Appleton (Charcoal: site title too dark, Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.13.52.08.mov

  • Attar (Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.13.53.49.mov

  • Dorna (Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.13.56.02.mov

  • Farrow (Charcoal: text too dark)
Screen.Recording.2023-04-11.at.13.57.29.mov

  • Heiwa (Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.13.59.32.mov

  • Meraki (Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.14.00.43.mov

  • Russell (Charcoal: low contrast text, Ruby wine: Same as Default)
Screen.Recording.2023-04-11.at.14.45.40.mov

  • Zoologist (White: Same as Default)
Screen.Recording.2023-04-11.at.14.49.07.mov

Some of the minor incompatibilities also happen without using headstart content on a self-hosted/local site

@pbking
Copy link
Contributor

pbking commented Apr 11, 2023

Consider instead this change for Jackson: D107566-code

The changes needed for Kingsley are a little more involved. The background of the cover block should be eliminated to allow the background color to bleed through, and the text in that block should use the default foreground colors. I'm not sure why/how the original content isn't doing that.

@pbking
Copy link
Contributor

pbking commented Apr 11, 2023

Also note: The reason why most of those other child themes look “the same” with “ruby wine” variation is because they are using only TWO colors while Blockbase is making available five. Thus those primary, secondary and tertiary color values, which are what make that variation special, aren’t used and are therefore not useful for those themes. (Including Jackson, but also many more and noted in github)

After tinkering with these child themes I’m not very keen on that choice (made looong ago) to have only two colors for many of these child themes. But I don’t know that changing it now is a good idea. I just don’t like it.

However including that style variation (Ruby Wine) for themes that only leverage two colors (foreground & background) ALSO doesn’t make much sense.

Until very recently none (most?) of these child themes had ANY style variations. Thus removing them made sense. But removing them via code for child themes was overly complicated and just removing them from Blockbase was done at the detriment of that theme.

I’m not sure what to do about it all. I just wanted to get those thoughts down.

We might also want to either REMOVE Ruby Wine (since it’s so un-different for so many themes) or change it in a more dramatic way (for instance, set a slightly red background color or something).

@mmtr
Copy link
Member Author

mmtr commented Apr 12, 2023

Until very recently none (most?) of these child themes had ANY style variations. Thus removing them made sense. But removing them via code for child themes was overly complicated and just removing them from Blockbase was done at the detriment of that theme.

IMO, Core's decision to expose style variations from parent themes to child themes was a bit drastic since it can have negative consequences for child themes that have been built with the (until then correct) expectation that they won't have any other style variations than the ones designed specifically for them.

It should have been an opt-in behavior. Or at the very least, provide an easy way to opt out.

That's why I think we should hide the style variation from Blockbase child themes. And I realize the code needed for that is complex (I wish I could do something simpler) but Core doesn't give us any other way.

EDIT: In f51deda, I updated the logic to use an approach that is slightly less complicated.

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

Successfully merging this pull request may close these issues.

3 participants