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

Add link color component #22722

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 28, 2020

Depends on: #22744
This PR adds link color picking UI and the ability to configure link color using theme.json.

The following blocks get a filed to pick the link color:

  • Paragraph
  • Heading
  • Group
  • Columns
  • Media Text

When the variable from theme.json is changed the UI and the new default link color is updated.

This PR has an issue it may change the default link color for existing themes. I don't think we can avoid that, the only option would be to not show the link color UI at all if the theme does not use theme.json

How has this been tested?

I verified the default link color is #00e and the UI of the blocks shows that.
I added the following code data to :

	"core/paragraph": {
		"styles": {
			"color": {
				"link": "red"
			}
		}
	}

I verified the default link color of the paragraph starts being red while for other blocks is still "#00e".
I tried to change colors in groups and verified the colors propagate correctly to the descends.

@github-actions
Copy link

github-actions bot commented May 28, 2020

Size Change: +353 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 106 kB +259 B (0%)
build/block-library/index.js 126 kB +32 B (0%)
build/block-library/style-rtl.css 7.72 kB +31 B (0%)
build/block-library/style.css 7.72 kB +31 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.75 kB 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-editor/style-rtl.css 11.4 kB 0 B
build/block-editor/style.css 11.4 kB 0 B
build/block-library/editor-rtl.css 7.87 kB 0 B
build/block-library/editor.css 7.88 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 193 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.25 kB 0 B
build/edit-navigation/style-rtl.css 918 B 0 B
build/edit-navigation/style.css 919 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/index.js 15 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/index.js 8.83 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.7 kB 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.3 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch 3 times, most recently from 57c14fc to 1ae7dbd Compare May 29, 2020 14:41
@jorgefilipecosta jorgefilipecosta changed the base branch from master to update/make-global-styles-shape-consitent-with-local-styles-shape May 29, 2020 14:42
@jorgefilipecosta jorgefilipecosta marked this pull request as ready for review May 29, 2020 14:49
@jorgefilipecosta jorgefilipecosta added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label May 29, 2020
@youknowriad youknowriad added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label May 29, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the update/make-global-styles-shape-consitent-with-local-styles-shape branch 4 times, most recently from a415c69 to b60733d Compare May 29, 2020 16:35
Base automatically changed from update/make-global-styles-shape-consitent-with-local-styles-shape to master May 29, 2020 16:53
@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from 1ae7dbd to 79a8437 Compare May 29, 2020 17:05
@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch 2 times, most recently from fb5c964 to d35b123 Compare June 2, 2020 16:15
@oandregal
Copy link
Member

👋 Jorge. This is great work put together very quickly, appreciate it! ❤️ 🌟


There's a few things in this PR, I'm trying to address them separately so feedback is actionable.

1. How user modifications at the block-level should work?

I believe we have a few options here

  • We bind the color value directly to the block variable (--wp-color-link: <css value>):

    • when user changes themes, the block style will persist user's choices
    • when user changes the color palette within the same theme (ex: set --my-theme-valiable to hotpink) the block style will be unaffected
  • We bind the theme variable to the block variable (--wp-color-link: var(--my-theme-variable)):

    • when the user changes themes, the block style is broken because the variable is not in scope
    • when the user changes the color palette within the same theme (ex: set --my-theme-variable to hotpink) the block style will be affected as well
  • We could also explore doing this in the same way text and background colors work: classes for preset, inline for custom values, which has the same trade-offs:

    • when user changes themes, the block style is broken because the classes are not in scope
    • when the user changes the color palette within the same theme (ex: set --my-theme-variable to hotpink) the block style will be affected as well

I think we need to build a common understanding about how this should work to have a direction to walk towards. In the conversations we had at the beginning of the year, my understanding was that user choices at the block level should persist theme switches and should take precedence over theme styles (global-level). Following that thinking, it seems that we should implement the first option.

2. How do you see link color statuses (hover, visited, active) fit into this picture?

I'm not sure how these fit with what we currently have (these can't be expressed as inline styles). I also wonder how this and the first point play together. @ItsJonQ has brought up this a few times, so pulling him here in case he has any suggestions.

3. Can we decouple block-level & global-level for this PR?

I see this connects both levels using __experimentalGlobalStyles to make sure editor controls (link color) reflect global-level styles. We need this feature! The work you've done here is great. However, do we need this now?

I suggested before that we should delay this connection until the pieces were mature, so the Global Styles feature can remain experimental for WordPress 5.5. Doing this now is forcing us to open-up Global Styles to production without having given theme authors time to give feedback, and we act on it. As of today, I feel we should keep Global Styles experimental for WordPress 5.5.

4. Should we normalize the CSS variable names?

The other variables we use follow this pattern --wp--preset--{preset-category}--{preset-value}. Following that pattern, does it make sense that this new variable goes like --wp--style--{style-category}--{style-value}, so it'd be --wp--style--color--link?


This needs rebase to make it work with block.json supports (as well as global supports).

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Jun 4, 2020

Hi @nosolosw, thank you for reviewing this PR and sharing important questions!

  1. How user modifications at the block-level should work?

This discussion is how to reference predefined colors. The same discussion was also happening at #21490 (comment). I will share my thoughts here.

I agree that there are three options as @nosolosw shared: Direct value, reference a CSS var, or class.
Direct value, to me, seems like an excluded option. We want to allow users to globally change colors across a website even for existing blocks in all posts/areas direct value does not offer that.

Regarding this disadvantage, "when the user changes themes, the block style is broken because the variable is not in scope". If predefined colors are kept and the theme is changed, the old colors may not look good at all on the new theme, and the design may appear broken as well. There may be cases where the design may look even more broken if predefined colors are kept than if the defaults (no color selected) of the new theme are applied.
I imagine given that theme.json/"global styles" allow use customization core or plugins may even offer an option to keep the presets of the old theme in user customizations when changing themes if desired.

This PR uses CSS vars because, in previous discussions, it seemed we decided to change the direction from class usage to CSS variable usage. It was this change of direction that made me propose #21490 to follow the new direction.
I don't have a strong preference between CSS variables references and classes. Each one has its advantage and disadvantages I think we need to choose a path and commit to it.
If we prefer to use CSS classes instead, I will change this PR to comply with the decision.
cc: @youknowriad as you have been involved in the conversations on #21490 (comment).

  1. How do you see link color statuses (hover, visited, active) fit into this picture?

This is a tough question. It relates to the discussion of variable reference vs. classes.
If we use classes hover, visited, active etc can be changed with CSS using something like:

.has-link-color.has-yyyy-link-color a:hoover {
	color: zzz;
}

If we use variables, there are not many options available. There are ways to use calc and compute a color based on other colors, but it would only work if we use variables for red, green, blue, or luminosity, saturation, etc. and making themes set colors in this way is not practical.
So it seems the options we have with variables are having a syntax in theme.json for these pseudo-classes, e.g., core/paragraph:hover or having specific variables

style: {
	color: {
		link:hover: yyy;
	}
}

At first sight, the pseudo-elements in theme.json make more sense. And I don't see a blocker for the theme.json engine to support them.
They seem like a natural solution to this problem. It also brings the question if the UI of block/global styles will indicate that there are hover, visited, etc. colors to the users, and if the UI will allow user to change them or not.
These challenges are not specific to link color any style even color may need to be changed using pseudo-classes, and it seems we may have a path forward using a syntax in theme.json. So I think we should not block this PR.
Cc: @matias, @youknowriad in case you have any thoughts related to what I shared above.

  1. Can we decouple block-level & global-level for this PR?

I see this connects both levels using __experimentalGlobalStyles to make sure editor controls (link color) reflect global-level styles. We need this feature! The work you've done here is great. However, do we need this now?

I think this piece is even more important than the link color itself and allows us to test global styles in ways that were not tested before. E.g., Have the blocks show default values that come from theme.json have the blocks show values inherit from the parents, etc.
It was with this implementation that I become aware of the need to have blocks know if their current instance is affected by a selector, e.g., know if the current heading is affected by core/heading/h1, and I also become aware of the inconsistency in shape between style attribute and theme.json that was meanwhile fixed with a PR extracted from this one. I think without having this system in practice, we are not totally aware of the challenges that will appear. So I think we should merge this PR and this mechanism as soon as possible, and then offer a way for the users to change the global style for link color on edit-site right after so we can have an end to end implementation of a global style.

I suggested before that we should delay this connection until the pieces were mature, so the Global Styles feature can remain experimental for WordPress 5.5. Doing this now is forcing us to open-up Global Styles to production without having given theme authors time to give feedback, and we act on it. As of today, I feel we should keep Global Styles experimental for WordPress 5.5.

Global styles are experimental. The files have the experimental prefix, it is common that we merge experimental features in core, so I don't see why global styles should be different and can not have experimental functionalities included.

As of today, I feel we should keep Global Styles experimental for WordPress 5.5.

I agree, and to me, it seems global styles are still experimental. The JSON files that set them have the experimental prefix. This PR is not adding anything to the public API that does not has the experimental flag (besides the link color style attribute) and was mostly implemented as a plugin. If we don't port the PHP files to the core that parse and include theme.json information, theme.json will not be able to change or affect the editor.
I think the only non-experimental public-facing part in this PR is the link color local block style attribute. But we can not have a link color picker without an attribute. We are already doing the same with line height etc. and including new style attributes. We may end up putting these parts of the UI behind a flag like the phase 2 flag if needed.

What in this PR does not looks experimental and should be fixed?

  1. Should we normalize the CSS variable names?

Yes, thank you for catching this. I think we should update the CSS variable name. I will change it to --wp--style--color--link.

@oandregal
Copy link
Member

  1. How user modifications at the block-level should work?

Although I have a preference for the first approach I shared, I understand that following any of the other options is how things work today, so I'd be fine merging this as it is. Perhaps when we have the global styles UI we can revisit this decision if it makes sense.

  1. How do you see link color statuses (hover, visited, active) fit into this picture?

👍 What you say makes sense, thanks for sharing. I agree it shouldn't stop this PR from being merged.

  1. Can we decouple block-level & global-level for this PR?

In this point, I believe we agree on the need to do this and that doing it will teach us valuable lessons. It seems we disagree when it comes to decide the time & space: whether or not we need it for this Pull Request.

I still think that a better space for testing the grounds of deep editor integration would be the global styles sidebar at the edit site. Note that this editor integration is not really required for the link color feature to work! If we decouple block & global levels, it'll work as fine as the other features we implemented and that we'll ship with WordPress 5.5 (colors, font sizes, line height). For example, we could do something like:

a {
  --wp--color--link: <value>;
  color: var(--wp--color--link);
}

In my view, it'd be a nice and scoped experiment (a CSS variable that is specific to links and scoped to a nodes), so doesn't leak to other places.

I reckon I may be conservative in this point, and I'll trust more experienced voices on this. I just want to make my rationale as clear as possible, which is hard in such a nuance topic: the reason I'm conservative is that I've seen that when something lands in WordPress core, it's really difficult to remove/update it (no matter the name starts or not with experimental). So, if something is not really necessary, should we allow ourselves to perhaps be a bit more prudent and wait to integrate this in the next WordPress cycle?

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from d35b123 to f5fd0f6 Compare June 4, 2020 16:16
@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from e5433e5 to fed69d4 Compare June 4, 2020 17:27
@jorgefilipecosta
Copy link
Member Author

Following a @nosolosw suggestion, this PR was updated to include a has-link-color class so we make sure we don't break the current link color for existing themes.
The class is only present if in fact a link color is selected.

When we allow link color to be set via theme.json we will need to stop relying on the class because in that case, a block may have a link color set in. theme.json without having the class in their markup.

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from fed69d4 to 59ce612 Compare June 4, 2020 17:43
@oandregal
Copy link
Member

I've tested this with 2020, 2019, and 2017 themes and this is working as expected:

  • blocks that have links but aren't styled by the user => color is whatever the theme was doing before
  • blocks that have links and are styled by the user with a preset link color => color is the user choice
  • blocks that have links and are styled by the user with a custom link color => color is the user choice

I think the only bit left would be to make this work with themes that do support theme.json. I've tested with https://github.com/nosolosw/global-styles-theme/pull/8

We could, for example, add in gutenberg_experimental_global_styles_resolver_styles something like this:

if ( gutenberg_experimental_global_styles_has_theme_json_support() ) {
	// To support all themes, we added in the block-library stylesheet
	// a style rule such as .has-link-color a { color: var(--wp--style--color--link, #00e); }
	// so that existing link colors themes used didn't break.
	// We add this here to make it work for themes that opt-in to theme.json
	// In the future, we may do this differently.
	$css_rule  = 'a { color: var(--wp--style--color--link, #00e); }' . "\n";
}

I think we can land this PR after that's addressed.

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from 59ce612 to 45cfd6f Compare June 4, 2020 18:54
@jorgefilipecosta
Copy link
Member Author

Hi @nosolosw thank you for the suggestion, I included that code.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Nice iteration here! This is working as the other attribute styles (color, background, font-size, and line-height): block level, global styles level, as well as for themes that use and don't use theme.json. 👏

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from 45cfd6f to ffe4f0b Compare June 4, 2020 21:40
@jorgefilipecosta jorgefilipecosta force-pushed the add/link-color-picking-in-global-styles branch from ffe4f0b to 784abc4 Compare June 5, 2020 07:58
@jorgefilipecosta
Copy link
Member Author

Thank you for all the reviews and insights @nosolosw!

@jorgefilipecosta jorgefilipecosta merged commit 8b949bf into master Jun 5, 2020
@jorgefilipecosta jorgefilipecosta deleted the add/link-color-picking-in-global-styles branch June 5, 2020 08:05
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 5, 2020
@oandregal
Copy link
Member

Created #22929 to document this feature.

@oandregal
Copy link
Member

@jorgefilipecosta now that global-styles.php needs to be loaded for every request, would you mind filling me up on what would we need to do to port this to WordPress core? Is this something that's done during the release or should we create a patch now?

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Jun 5, 2020

The backporting to core essentially is implementing the same functionality we have has a plugin into core.
We can create a patch now or before the release. We normally don't have a strict date on that. But the patch needs to be merged when the packages in core are updated to a package release that includes link color, otherwise link color will not work. It also needs to pass by all the core review processes which takes sometimes so depending on the task it can take some time.

When backporting things to core it does not means the functionality is implemented as in the plugin. E.g: instead of using a filter, in core it may make more sense to change directly the place where the filter is executed. The code should be equivalent to what we have in the plugin but should not be the same it needs to look like it was built in core first

@jorgefilipecosta
Copy link
Member Author

As referred in #22929 (comment). If we think we are not ready to have the global styles loading in the core we have some options.

  • Remove the CSS var mechanism for now, I think it is an undesirable solution because it will block testing the reference mechanism but it is an option.
  • Rely on process.env.GUTENBERG_PHASE and only use the reference mechanism on the plugin.

@oandregal oandregal changed the title Add link color picking in global styles Add link color component Jun 8, 2020
@oandregal oandregal added the [Type] Feature New feature to highlight in changelogs. label Jun 8, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
@ellatrix ellatrix mentioned this pull request Jul 3, 2020
12 tasks
@oandregal oandregal removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 15, 2020
@oandregal
Copy link
Member

As per #23210 (comment) this isn't going to be part of 5.5 and it requires the plugin to work.

Discussion about this has been scattered through issues and reviews, so I'm listing them here for clarity/awareness:

  • This needs the presets as CSS variables taken from theme.json/add_theme_support, which is not yet stable.
  • We need to consolidate how we store the style attributes across all style attributes. We may even need to change it to using classes if we want to support some interaction themes are doing now (ex: they change text colors when the background changes).

@mtias mtias mentioned this pull request Jul 29, 2020
82 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants