Skip to content
This repository has been archived by the owner on Feb 17, 2025. It is now read-only.

Seedlet: Allow all blocks with a link color to override the text color value set by custom background colors #2275

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Jul 20, 2020

Changes proposed in this Pull Request:

  • When using a custom background color that changes the text color (e.g. picking a black background sets the text white), only change the link color if it's not specified by the block.
  • Remove the override that only worked for blocks with p as root element (e.g. the Paragraph block).
  • Add a missing fallback color for the --wp--style--color--link variable.

This change originates from a discussion in Gutenberg, where we noticed that the theme's style specificity was getting in the way with the link colors on non-Paragraph blocks.

The missing fallback color was throwing a postcss-css-variable working when building the theme style.
The --wp--style--color--link variable is defined by Gutenberg, and it's supported by all current browsers.
For older browsers, the build system uses a fallback color.
Without providing one, the ie.css style showed something like this:

a:active { color: undefined; }

It's not very clear by the diff because the change was mixed up with the p override removal, but basically I just did this:

&:active {
-	color: var(--wp--style--color--link);	
+	color: var( --wp--style--color--link, var(--global--color-primary) );	
}	

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.

In my testing, this works for Headings but not Paragraphs:

Screen Shot 2020-07-20 at 2 55 24 PM

Screen Shot 2020-07-20 at 2 55 32 PM

@Copons
Copy link
Contributor Author

Copons commented Jul 21, 2020

In my testing, this works for Headings but not Paragraphs:

@jffng my bad, I've missed one more override 🤦

@Copons Copons force-pushed the update/link-color-overrides branch from cae0737 to f1ecc74 Compare July 21, 2020 10:34
@kjellr
Copy link
Contributor

kjellr commented Jul 21, 2020

Regarding the approach here, I originally tried using extra :not() rules instead of the extra specificity being deleted here, but at the time I found it to be pretty complicated and unwieldy to get right. Looking at that code fresh, I see I missed some use cases outside of the paragraph block though.

In any case, I just tested this PR briefly, but noticed that this doesn't cover all cases (as noted above, I don't think the old styles did either so I'm not sure that it's a regression). But this sample content should come in handy for testing purposes.

In all of these cases, the links labeled "custom blue link" should be blue:

dotorgthemes test_wp-admin_post php_post=2793 action=edit (2)

@Copons
Copy link
Contributor Author

Copons commented Jul 21, 2020

@kjellr thanks for sharing the sample content!
The combinations to test here are just so many and I really appreciate it! 🙇

@Copons
Copy link
Contributor Author

Copons commented Jul 21, 2020

Compared to your screen, headers with background look fine to me. 🤔

Screenshot 2020-07-21 at 15 41 28

On the other hand, all nested blocks fail when the container has a background but no link color, which is logic given how the selector works.

Maybe giving the double-not a more "positive" spin could be the answer.

@jeyip
Copy link
Contributor

jeyip commented Jul 21, 2020

I went ahead and recreated all of the blocks @kjellr made. Some of my results are different. 🤔


Screen Shot 2020-07-21 at 3 42 22 PM

  • I see the styling overwrites but couldn't match them to styles in Seedlet. I suspect it may be a simple fix since it looks fine in Jacopo's and Kjell's environment. Will search more tomorrow.

Screen Shot 2020-07-21 at 3 43 56 PM

  • I see the styling overwrites but couldn't match them to styles in Seedlet. I suspect it may be a simple fix since it looks fine in Jacopo's and Kjell's environment. Will search more tomorrow.

Screen Shot 2020-07-21 at 3 45 56 PM


Screen Shot 2020-07-21 at 3 46 30 PM


Screen Shot 2020-07-21 at 3 46 22 PM


@Copons
Copy link
Contributor Author

Copons commented Jul 22, 2020

Thanks for double checking @jeyip!
I should have clearly marked this PR as WIP.
I have a clear idea on how to solve almost all the issues, although I can't understand why in "unstyled" blocks, Guten's default link color (dark blue in the screenshots) manages to override the theme's link color.
Or to put it better, I understand why, but I haven't found a clean way to make the theme color's win over Guten's.

@Copons Copons force-pushed the update/link-color-overrides branch from f1ecc74 to 3e80f8e Compare July 22, 2020 11:11
@Copons
Copy link
Contributor Author

Copons commented Jul 22, 2020

It seems everything is working fine with my latest change.
(Filter by .scss when reviewing to conserve your sanity!)

One quirk I've noticed when setting a text (not link) color.
If the block doesn't have a background color (or it's the theme's background color), the link will use the actual link color specified by the theme.
Otherwise, it will inherit the text color.

Screenshot 2020-07-22 at 12 13 40

I'm pretty sure the idea there is to automatically adapt the text color according to the background (e.g. picking a black background will turn the text, link included, to white).

This creates a little inconsistency, but I'm kinda inclined to disregard it.
Any thoughts?

@jeyip
Copy link
Contributor

jeyip commented Jul 23, 2020

Testing

  • For Chrome, Firefox, Safari, and Edge, everything works as advertised except for the inconsistency that Jacopo has pointed out.
  • Side note: Styles don't seem to work out in IE11, but to be fair, everything looks borked there. If we are actively supporting IE11 for seedlet, then it seems like those issues might be better addressed in a separate PR. See example below.

This creates a little inconsistency, but I'm kinda inclined to disregard it.

Although this PR fixes a lot of link styling, I'm not sure how I feel introducing a small regression in behavior. I'm open to the idea, but I'm curious what Kjell thinks.


Screen Shot 2020-07-23 at 12 39 49 AM

@Copons
Copy link
Contributor Author

Copons commented Jul 23, 2020

Side note: Styles don't seem to work out in IE11, but to be fair, everything looks borked there. If we are actively supporting IE11 for seedlet, then it seems like those issues might be better addressed in a separate PR. See example below.

@jeyip Yeah, that's expected unfortunately.
These colors work with CSS variables which are not supported by IE11.

If you look at the generated ie.css file, all rules using a variable defined in Gutenberg (as, in this case, --wp--style--color--link) automatically use their fallback (typically a variable defined in the theme itself) instead of var( --wp--style--color--link )).

@jeyip
Copy link
Contributor

jeyip commented Jul 23, 2020

@jeyip Yeah, that's expected unfortunately.
These colors work with CSS variables which are not supported by IE11.

👍 Thanks for the clarification.

@Copons This might be a silly question, but could we add currentColor into the fallback assignment for the paragraph text? It seems like it would make sense given the class name has-text-color. I also did a quick verification. Adding the fallback does not affect automatic adaptation of the text color according to the background (e.g. black background will turn the text, link included, to white).

// Frontend
p.has-text-color a {
	color: var(--wp--style--color--link, currentColor, var(--global--color-primary));
}

// Editor
.wp-block a {
        color: var( --wp--style--color--link, currentColor, var(--global--color-primary) );
}

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 think the styles need to be recompiled and committed because some commented CSS is sneaking into the built styles. Other than that, this works as advertised in my testing.

Thank you!

@Copons Copons force-pushed the update/link-color-overrides branch from be762a5 to 7ec9617 Compare July 27, 2020 14:55
@Copons
Copy link
Contributor Author

Copons commented Jul 27, 2020

Good catch @jffng! I've rebased and rebuilt, and those comments disappeared. 🙂

@jffng
Copy link
Contributor

jffng commented Jul 27, 2020

Thank you @Copons @jeyip. I'm going to merge this because we want to more fully support custom link colors, and the addition of this PR covers more use cases than what we have now.

Also mentally bookmarking this PR as a good example of how challenging it is right now for a theme to support an increasing number of styling combinations provided by the editor. Hopefully the amount of highly targeted CSS provided by the theme can be reduced as global styles progresses.

@jffng jffng merged commit a12ef79 into master Jul 27, 2020
@jffng jffng deleted the update/link-color-overrides branch July 27, 2020 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants