Automattic / themes

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

Assembler: Remove inherited color styling from linked headings #8206

Closed ivan-ottinger closed 1 month ago

ivan-ottinger commented 1 month ago

Fixes https://github.com/Automattic/themes/issues/8193.

Changes proposed in this Pull Request:

There doesn't seem to be a need for the !important inherited color override that is causing small regression with the hover link color.

Testing instructions:

  1. Add Title and Heading blocks to a page / post and make sure these titles are linked somewhere (# should be enough).
  2. Try changing blocks' colors, including hover color through the editor sidebar.
  3. Adjust link and text colors through the Global Styles.
  4. There should be no regressions or unexpected color styling on the published page / post.
  5. Try to reproduce https://github.com/Automattic/themes/issues/8193. It shouldn't be possible.

Related issue(s):

github-actions[bot] commented 1 month ago

Preview changes

I've detected changes to the following themes in this PR: Assembler. You can preview these changes by following the links below:

I will update this comment with the latest preview links as you push more changes to this PR. ⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

allilevine commented 1 month ago

Changing the link color and hover color worked for me in a post, but I'm not seeing the hover color when editing a template:

Screen Shot 2024-09-26 at 9 16 13 AM

Is that expected?

ivan-ottinger commented 1 month ago

Changing the link color and hover color worked for me in a post, but I'm not seeing the hover color when editing a template:

Thanks for catching that, Allison! Keeping the color inheritance in and removing just the !important as Luis suggested above fixes that as well. πŸ™‚

ivan-ottinger commented 1 month ago

Thank you for additional review, Allison!

ivan-ottinger commented 1 month ago

Hey @Automattic/theam πŸ‘‹πŸΌπŸ™‚ Just wanted to check if we can merge this PR - or - whether you will be merge and deploying it later?

(I already read the docs here: https://github.com/Automattic/themes/blob/trunk/CONTRIBUTING.md, but haven't contributed to this repo yet)

Thanks!

mikachan commented 1 month ago

Thanks for the ping, @ivan-ottinger! Feel free to merge this PR whenever you're ready. As this is a dotcom theme, either @alaczek or @miksansegundo would usually handle the deployment, but I'm also happy to help deploy.

richtabor commented 1 month ago

Let me double check; I'm trying to recall why it was added in the first place.

richtabor commented 1 month ago

I actually think we can just remove the whole bit, as link color set to currentColor like this. I'm pretty sure this was for some specificity issue, but that has since been resolved.

github-actions[bot] commented 1 month ago

Theme-Check results


assembler: No changes required βœ….


ivan-ottinger commented 1 month ago

This is working well for me now! I see the hover colors while editing and they show up correctly in the published post. The hover color is also visible when editing global styles. βœ…

Hey @allilevine πŸ‘‹πŸΌ Interestingly, all is working well even with the whole color inheritance gone. I think it must have been some kind of glitch before.

The only issue I noticed (not related to this PR, nor the theme either) is that when the Title block is added when editing templates, the anchor isn't being added to the markup:

Markup on 2024-09-27 at 09:03:00

As a result, the styles don't get applied.

It is not clear to me if this is a bug or done on purpose for some reason, so I opened a new issue in the Gutenberg repo: https://github.com/WordPress/gutenberg/issues/65691


Thank you for the additional review and merge @mikachan, @alaczek and @richtabor!

For completeness, here's the difference between having color inheritance in vs not:

with inheritance - heading link color is inherited from the text color:

Markup on 2024-09-27 at 08:41:53

without inheritance - actual link color is being used:

Markup on 2024-09-27 at 08:43:23