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

Conversation

@jffng
Copy link
Contributor

@jffng jffng commented Feb 9, 2021

This PR addresses #2566.

Tested with and without color annotations on the Mayland theme, and appears to work fine.

My hesitation in adding this is that there is no support yet for setting the link's hover color in Gutenberg, so may result in some strange combinations.

@jffng
Copy link
Contributor Author

jffng commented Feb 9, 2021

At first I thought I needed to do this, but it's unnecessary because Gutenberg handles applying the custom link styles cleanly.

@jffng jffng linked an issue Feb 9, 2021 that may be closed by this pull request
@pbking
Copy link
Contributor

pbking commented Feb 9, 2021

According to the doc (and mirroring the work in spearhead) I believe the experimental-theme.json will need to be added for this to work correctly. (Otherwise I believe the link colors will not be colored correctly by default.)

Which I think will mean that this change will require adding experimental-theme.json to ALL of the Varia children.

@jffng
Copy link
Contributor Author

jffng commented Feb 10, 2021

If a theme opts in, it should define default link colors in experimental-theme.json (or in its theme styles if no experimental-theme.json is present)

It should work without the theme.json, did you try it?

@jffng jffng requested a review from a team February 10, 2021 14:28
@scruffian
Copy link
Member

I think we need the theme.json to set a default color for links...

Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@jffng
Copy link
Contributor Author

jffng commented Feb 10, 2021

After testing more use cases, I think we should only add this theme support to themes that have implemented CSS variables — which is only Hever to my knowledge.

My reasoning is because Gutenberg makes use of CSS variables (e.g. --wp--preset--color--primary) to apply custom link colors when a preset color is chosen in the editor. If we add the flag to Varia, custom link styles would not be applied correctly in any child theme without those preset values — either coming from theme.json or its stylesheet. And I don't think we should endeavor to add those variables via this PR.

Thoughts @scruffian @pbking @MaggieCabrera ?

@scruffian
Copy link
Member

So what would happen in other themes? Couldn't we just add theme.json to all the child themes?

@jffng
Copy link
Contributor Author

jffng commented Feb 10, 2021

Couldn't we just add theme.json to all the child themes?

I think this would break link colors for themes using a custom palette / color annotations.

I'm having trouble getting this to work on dotcom, but it is working locally. It may have to do with the changes to the shape of the theme.json.

@jffng jffng force-pushed the try/varia-add-experimental-link-color branch from 164ffaa to 04047d6 Compare February 10, 2021 21:21
@pbking
Copy link
Contributor

pbking commented Feb 11, 2021

Works fine locally.
Doesn't work as expected on .com (yet, until Gutenberg 9.9 is available there)

@scruffian
Copy link
Member

Moving this to blocked while we wait for 9.9 to launch on wpcom

@jjchrisdiehl
Copy link

Moving this to blocked while we wait for 9.9 to launch on wpcom

@scruffian It looks like 9.9 was updated on atomic sites, is this change likely to be merged soon?

@scruffian
Copy link
Member

This was done in #3327

@scruffian scruffian closed this Feb 25, 2021
@scruffian scruffian deleted the try/varia-add-experimental-link-color branch February 25, 2021 18:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Varia Themes: Add support for custom link color

5 participants