Automattic / themes

Free WordPress themes made by Automattic for WordPress.org and WordPress.com.
https://themeshaper.com
GNU General Public License v2.0
883 stars 351 forks source link

Maywood: Add a new Cover block style variation that's without text shadow #1837

Closed iamtakashi closed 4 years ago

iamtakashi commented 4 years ago

Maywood adds text-shadow for the text on Cover block to help the text stands out a bit, but it's been come up as an issue more than once. p9Jlb4-1g4-p2 and https://github.com/Automattic/wp-calypso/issues/39350.

As it's been suggested by @kjellr, it'd be great if we can add a new cover block style that's without the shadow, and make it as the default style so that the change wouldn't be so disruptive

alaczek commented 4 years ago

make it as the default style so that the change wouldn't be so disruptive

I might be wrong here, but I think changing the default would be disruptive, because customers would have to go back and switch the style to add back the shadow (previous default).

Wouldn't adding the block style be enough for our purposes anyway? AFAIK we can pre-select block style for the page layouts.

iamtakashi commented 4 years ago

To be honest, I'm not entirely sure how the default work :) I imagined it won't change the style of published contents, but I might be totally wrong on that. If so, yes, adding a new style alone would be enough.

iamtakashi commented 4 years ago

@alaczek Thanks for working on a PR! I played around with your PR and I can see now that with isDefault: true, the block selects the new style as default but it doesn't add the CSS class for the style. The behavior matches with the handbook.

By adding isDefault: true you can mark the registered style variation as the one that is recognized as active when no custom class name is provided. It also means that there will be no custom class name added to the HTML output for the style that is marked as default.

But then, if I'm not wrong, we're going to have to manually add the class to all cover block on all the existing and future templates, aren't we? I'm not sure if that realistic.

We might have to remove the shadow from the default and add a variation with text-shadow instead. It's going to disruptive, but at least, the customers will have a way to add back the shadow if they want. What do you think?

cc @kjellr

ianstewart commented 4 years ago

Here's an example with one of our WP.com starter design layouts used in Maywood.

image

Vs Balasana …

image

iamtakashi commented 4 years ago

What about removing the shadow if there is no image in a cover block? That'd still change users sites, but it won't be as disrupting as removing it completely.

kjellr commented 4 years ago

We actually have another PR over at #1807 that I think does the same thing as #1839. It got lost in the shuffle at some point, but just needs to be merged in + deployed.

What about removing the shadow if there is no image in a cover block? That'd still change users sites, but it won't be as disrupting as removing it completely.

Regarding this, yes, I think that makes sense. Unfortunately, it does't look like there's any difference in the classes added when a cover block does or does not have an image assigned, so I'm not sure how we'd make that change. 🤔

The top here has just a color background, and the bottom one has an image background (plus the color overlay, as usual):

Screen Shot 2020-04-29 at 9 34 16 AM
iamtakashi commented 4 years ago

@kjell, thanks for the info!

Unfortunately, it does't look like there's any difference in the classes added when a cover block does or does not have an image assigned, so I'm not sure how we'd make that change.

How about something like this? .wp-block-cover[style*="background-image"] ... { text-shodow: none }

kjellr commented 4 years ago

Targeting the style attribute is weird, but it works. 😄

I pushed #1949, which takes that approach, and also ensures that the text shadow stays if there's a video background.

allancole commented 4 years ago

This is fixed in #1949 which I just merged to master and deployed to Dotcom.

Closing this issue 👍