-
Notifications
You must be signed in to change notification settings - Fork 357
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
Conversation
Preview changesI've detected changes to the following themes in this PR: Assembler. I will update this comment with the latest preview links as you push more changes to this PR. |
/* Make link colors match text colors. */ | ||
|
||
h1 a, h2 a, h3 a, h4 a, h5 a, h6 a { | ||
color: inherit !important; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks for catching that, Allison! Keeping the color inheritance in and removing just the |
There was a problem hiding this 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. ✅
Thank you for additional review, Allison! |
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! |
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. |
Let me double check; I'm trying to recall why it was added in the first place. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Theme-Check resultsassembler: No changes required ✅. |
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: 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:without inheritance - actual link color is being used: |
Fixes #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:
Title
andHeading
blocks to a page / post and make sure these titles are linked somewhere (#
should be enough).Related issue(s):