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

Seedlet Blocks: Fix block-level global styling for site title block #4446

Merged

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Aug 20, 2021

Changes proposed in this Pull Request:

Site title block level global styling text color changes are being overridden because of a specificity conflict. To elaborate:

  • Block level global styles are being applied to the site title block through .wp-block-site-title a
  • The Seedlet blocks theme has a competing declaration via .is-root-container a:not(.wp-block-button__link):not([download])

I'm curious how folks feel about this PR. To keep the surface area of changes small, I tried piggybacking on the :not pseudo-selector convention. Rather than tinkering with increasing or decreasing specificities, I include the site title block as another element not to select for the seedlet blocks selector. In doing so, the user customized global style always wins out.

I didn't see any obvious visual regressions.

Before

2021-08-16 13 59 55

After

2021-08-20 03 11 51

Related issue(s):

#4447

@jeyip jeyip added [Type] Bug Something isn't working [Theme] Seedlet Blocks labels Aug 20, 2021
@jeyip jeyip self-assigned this Aug 20, 2021
@@ -148,7 +148,7 @@
/**
* Links in Content
*/
.is-root-container a:not(.wp-block-button__link):not([download]),
.is-root-container a:not(.wp-block-button__link):not([download]):not([aria-label="Site title text"]),
Copy link
Contributor Author

@jeyip jeyip Aug 20, 2021

Choose a reason for hiding this comment

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

Note:

Using an aria-label attribute as an identifier seems no bueno, but I couldn't think of a better way to handle the specificity conflict. I didn't see any other obvious ways of identifying the site title a tag.

@pbking pbking force-pushed the fix/seedlet-blocks-overriding-site-title-block-global-styles branch from 72775c2 to 72768e1 Compare August 20, 2021 15:50
@pbking
Copy link
Contributor

pbking commented Aug 20, 2021

@jeyip changes made to Blockbase themes need to happen in the .scss file (there's not much CSS that doesn't go through the scss compiler).

I tinkered with this PR a big and would appreciate your eyes on it.

I also found that the :not()'s that were already there don't seem to be needed if we just take out the color attribute. As far as I can tell that shouldn't have been there in the first place so I have removed it as well as cleaned out a few other related bits to clean that up.

I also found that the site-title in the content area was being handled differently than what's in a header template part. Pay careful attention to that when taking a look at this change please. I think it's good now.

I tested changing the colors of the site-title block (globally) as well as link color (globally) to make sure that it was doing what I expected it to do.

image

@jeyip
Copy link
Contributor Author

jeyip commented Aug 20, 2021

changes made to Blockbase themes need to happen in the .scss file (there's not much CSS that doesn't go through the scss compiler).

Thank you! That makes total sense.

I tinkered with this PR a big and would appreciate your eyes on it.

I also found that the site-title in the content area was being handled differently than what's in a header template part. Pay careful attention to that when taking a look at this change please. I think it's good now.

I tested changing the colors of the site-title block (globally) as well as link color (globally) to make sure that it was doing what I expected it to do.

Taking a look now. Thanks again for the guidance here 🙏

@jeyip
Copy link
Contributor Author

jeyip commented Aug 20, 2021

I saw one potential styling issue where the hover states of links are different from the site title. It's made more apparent if we set a global link color.

2021-08-20 11 36 15

We remove the color css declaration in _link.scss on hover and focus. Should we do the same for _site-title.scss? Happy to make the changes for us if you agree. Besides that, your updates generally look good to me.

@jeyip
Copy link
Contributor Author

jeyip commented Aug 20, 2021

While testing, I also saw an issue unrelated to your changes. I noticed that the site title in the header has a hard-coded inline styled font-size, which is preventing us from modifying the block-level global styling for font size.

<!-- wp:site-title {"textAlign":"center","style":{"typography":{"fontSize":"55px"}}} /-->

Edit:
Nevermind! The unrelated issue I thought I saw here is actually working as expected. The hard-coded inline font size creates a block setting font size selection for site title. When a block setting competes with block-level global styles, it should always override global styles.

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

I gave this a spin and here's what I found. Caveat that I am not sure what the expected behavior should be.

  • Changing the global text color of the Site Title block does not change the color
  • Changing the global link color of the Site Title block does change the color (as shown here)
  • The hover state for the site title does not respect the global color customizations. Maybe this line should be removed?

@jeyip
Copy link
Contributor Author

jeyip commented Aug 23, 2021

  • Changing the global text color of the Site Title block does not change the color
  • Changing the global link color of the Site Title block does change the color

@jffng From what I've seen, this brings the site title block to parity with other blocks. For example, the Post Comments Link Block and the Query Pagination Block behave similarly to what you're describing.

I do agree that the behavior doesn't seem ideal though. How do you feel about moving forward with this PR to get the site title block working and I can make an issue in core Gutenberg to discuss the global styles user experience further?

  • The hover state for the site title does not respect the global color customizations. Maybe this line should be removed?

Great minds think alike 🧠

I saw the same issue and mentioned the same potential solution in this comment. I'll go ahead and update the PR in a bit so that the behavior is more consistent.

@jeyip
Copy link
Contributor Author

jeyip commented Aug 23, 2021

The hover state for the site title does not respect the global color customizations. Maybe this line should be removed?

Made the changes in 257494b

Lemme know what y'all think 👍

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

@jeyip I appreciate you working on this! I am a bit torn about the change. This PR fixes global color customizations for all links, including the site title. So from that perspective the changes make sense.

However I don't like that we have to degrade the hover states a bit for all links. I don't think there's a way around this, since we do not yet have a way via global styles to handle hover states for buttons or links. So weighing those two things, I'm going to give a ✅ and hopefully there is a way to handle this sooner than later.

@jeyip jeyip merged commit c3be92a into trunk Aug 24, 2021
@jeyip jeyip deleted the fix/seedlet-blocks-overriding-site-title-block-global-styles branch August 24, 2021 00:44
@jeyip
Copy link
Contributor Author

jeyip commented Aug 25, 2021

I gave this a spin and here's what I found. Caveat that I am not sure what the expected behavior should be.

Changing the global text color of the Site Title block does not change the color
Changing the global link color of the Site Title block does change the color (as shown here)

Quick follow-up to this strange behavior @jffng. I created a PR that should address this. There was a bug where, even though blocks were explicitly disabling color and background color supports, they were still being displayed in the block-level global styles UI. WordPress/gutenberg#34293

In the case of the Site Title block, text colors should be disabled. https://github.com/WordPress/gutenberg/blob/599b60d2cc8b26b9bc2141305cfe5e07b23dab88/packages/block-library/src/site-title/block.json#L20-L23

When the aforementioned bug fix is merged, text color options should no longer be displayed in the UI.

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

Successfully merging this pull request may close these issues.

3 participants