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

Assembler: Remove inherited color styling from linked headings #8206

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

ivan-ottinger
Copy link
Contributor

@ivan-ottinger ivan-ottinger commented Sep 26, 2024

Fixes #8193.

Changes proposed in this Pull Request:

  • Assembler: Remove inherited color styling from linked headings

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 [HC] Assembler: Link hover color not updating on Post titles #8193. It shouldn't be possible.

Related issue(s):

@ivan-ottinger ivan-ottinger self-assigned this Sep 26, 2024
Copy link
Contributor

github-actions bot commented Sep 26, 2024

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.

/* Make link colors match text colors. */

h1 a, h2 a, h3 a, h4 a, h5 a, h6 a {
color: inherit !important;
Copy link
Contributor

@zaguiini zaguiini Sep 26, 2024

Choose a reason for hiding this comment

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

What happens if we remove the !important but keep the inheritance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Luis. Good point. I did not observe any issues when we remove the !important. It might be a safer approach to keep the color inheritance in place. → Added it back in the latest commit.

@allilevine
Copy link
Member

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
Copy link
Contributor Author

ivan-ottinger commented Sep 26, 2024

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. 🙂

Copy link
Member

@allilevine allilevine left a comment

Choose a reason for hiding this comment

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

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. ✅

@ivan-ottinger
Copy link
Contributor Author

Thank you for additional review, Allison!

@ivan-ottinger
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

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

@richtabor
Copy link
Contributor

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.

Copy link
Contributor

@richtabor richtabor left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Theme-Check results

assembler: No changes required ✅.


@alaczek alaczek merged commit 4a0b95a into trunk Sep 27, 2024
2 checks passed
@alaczek alaczek deleted the fix/assembler-heading-color-styles branch September 27, 2024 02:38
@ivan-ottinger
Copy link
Contributor Author

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: WordPress/gutenberg#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groundskeeping Worked on by Dotcom Groundskeeping [Theme] Assembler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HC] Assembler: Link hover color not updating on Post titles
6 participants