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

added outline support for blocks via theme.json #43526

Merged
merged 2 commits into from
Sep 7, 2022
Merged

Conversation

MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Aug 23, 2022

What?

This PR adds support for outline for blocks and elements via theme.json

Why?

This is something themes have to add most of the time using CSS. We should discuss what's the best way to surface this to the UI and which blocks should show the option, but the first step is here to allow themes to set it using theme.json instead of having to write CSS for it.

How?

I implemented this following the lead of #41972

Testing Instructions

I'm testing TT3, with this in theme.json:

"elements": {
	"button": {
		"outline": {
			"offset": "3px",
			"width": "3px",
			"style": "dashed",
			"color": "red"
		},
		":hover": {
			"outline": {
				"offset": "3px",
				"width": "3px",
				"style": "solid",
				"color": "blue"
			}
		}
	}
}

Screenshots or screencast

Screen Capture on 2022-08-23 at 16-45-03

/cc @WordPress/block-themers

@MaggieCabrera MaggieCabrera self-assigned this Aug 23, 2022
@MaggieCabrera MaggieCabrera added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Aug 23, 2022
@mikachan mikachan linked an issue Aug 23, 2022 that may be closed by this pull request
Copy link
Contributor

@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

Comment on lines +12 to +13
path: string[] = [ 'outline', 'color' ],
ruleKey: string = 'outlineColor'
Copy link
Contributor

@ajlende ajlende Aug 24, 2022

Choose a reason for hiding this comment

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

I think these two arguments are for box model generation (top, right, bottom, left) which outline doesn't have. They should probably be removed here and passed directly to the generateRule function.

@ramonjd am I correct in thinking that? Or does this not matter?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping!

I think these two arguments are for box model generation (top, right, bottom, left) which outline doesn't have.

While they share similar arguments, box model generation should use generateBoxRules rather than generateRule.

Example is margin. Note the margin%s pattern for individual sides:

const margin = {
	name: 'margin',
	generate: ( style: Style, options: StyleOptions ) => {
		return generateBoxRules( style, options, [ 'spacing', 'margin' ], {
			default: 'margin',
			individual: 'margin%s',
		} );
	},
};

So I think this looks okay.

They should probably be removed here and passed directly to the generateRule function.

I don't think it matters in practice, but in theory it might be better to be explicit about the arguments passed to generateRule, rather than set them as defaults.

There is little risk of it I'd say, but it would prevent unexpected path and ruleKey values being pass to the generate functions.

See how typography is set up, e.g., :

const fontSize = {
	name: 'fontSize',
	generate: ( style: Style, options: StyleOptions ) => {
		return generateRule(
			style,
			options,
			[ 'typography', 'fontSize' ],
			'fontSize'
		);
	},
};

Copy link
Contributor

@ajlende ajlende Aug 25, 2022

Choose a reason for hiding this comment

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

@ramonjd Thanks for sharing!

I was able to do some refactoring to remove the dependence on those two arguments to enforce consistency between all of the styles. Details are in #43594.

If we make the changes to be more like typography there wouldn't be any conflicts with it in this PR.

@ramonjd
Copy link
Member

ramonjd commented Aug 25, 2022

We should discuss what's the best way to surface this to the UI and which blocks should show the option

Let us know when this happens as we'll probably need to support outline in the style engine PHP classes as well. 👍


Not related to this PR (also on trunk), but it exposes how core classes are called before the extended Gutenberg classes.

I see this PHP notice in the editor:
Notice: Undefined index: button in /var/www/html/wp-includes/class-wp-theme-json.php on line 1482

The core Theme JSON class is trying to parse element selectors but button doesn't yet exist on the whitelist.

@colorful-tones
Copy link
Member

This would be a great addition. I'm not sure I'll have time to test though. Just leaving feedback 😄

@scruffian scruffian merged commit 40efcbe into trunk Sep 7, 2022
@scruffian scruffian deleted the try-outline-property branch September 7, 2022 11:12
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 7, 2022
import spacing from './spacing';
import typography from './typography';

export const styleDefinitions = [
...border,
...color,
...outline,
Copy link
Member

@ramonjd ramonjd Sep 7, 2022

Choose a reason for hiding this comment

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

These changes need an Unreleased change log entry, e.g.,

## New features 

- Adding outline support in the Style Engine JS package [#43526](https://github.com/WordPress/gutenberg/pull/43526)

@cbravobernal cbravobernal added the [Type] Enhancement A suggestion for improvement. label Sep 13, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 20, 2022
@femkreations femkreations removed the Needs User Documentation Needs new user documentation label Sep 25, 2022
ramonjd added a commit that referenced this pull request Sep 28, 2022
Adding unit tests for outline properties
ramonjd added a commit that referenced this pull request Sep 28, 2022
* Follow up for #44502 and #43526
Adding unit tests for outline properties

* Property order
oandregal added a commit that referenced this pull request Dec 15, 2022
@oandregal
Copy link
Member

Hey, while working on #46579 I realized this PR was not backported to WordPress 6.1. When we land that PR and then backport to WordPress 6.2 everything will be fine, but I wanted to raise in case you think this should be ported for a potential WordPress 6.1.X release.

@MaggieCabrera
Copy link
Contributor Author

MaggieCabrera commented Dec 19, 2022

Thanks for letting me know @oandregal ! I don't know how urgent this one would be. I believe TT3 did not implement this but some other themes like Blockbase, Block Canvas... do have it.

I created a backport PR in any case WordPress/wordpress-develop#3788

@MaggieCabrera
Copy link
Contributor Author

as it was mentioned on my PR, this change would benefit from some unit tests, but I don't think I can get to that before my vacation starts. If adding this is to a point release is time sensitive, I'm happy to wait until we can add those tests.

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Dec 19, 2022
Adds the ability to define outline CSS properties for elements and blocks within `theme.json` to render `outline-color`, `outline-offset`, `outline-style`, and `outline-width` styles.

Originally developed and tested in [WordPress/gutenberg#43526 Gutenberg PR 43526].

Props onemaggie, hellofromTonya, audrasjb, ironprogrammer.
Fixes #57354.

git-svn-id: https://develop.svn.wordpress.org/trunk@55008 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Dec 19, 2022
Adds the ability to define outline CSS properties for elements and blocks within `theme.json` to render `outline-color`, `outline-offset`, `outline-style`, and `outline-width` styles.

Originally developed and tested in [WordPress/gutenberg#43526 Gutenberg PR 43526].

Props onemaggie, hellofromTonya, audrasjb, ironprogrammer.
Fixes #57354.
Built from https://develop.svn.wordpress.org/trunk@55008


git-svn-id: http://core.svn.wordpress.org/trunk@54541 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@hellofromtonya
Copy link
Contributor

Update: The backport (see Core Trac 57354) has been committed into Core with a unit test ✅

github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Dec 19, 2022
Adds the ability to define outline CSS properties for elements and blocks within `theme.json` to render `outline-color`, `outline-offset`, `outline-style`, and `outline-width` styles.

Originally developed and tested in [WordPress/gutenberg#43526 Gutenberg PR 43526].

Props onemaggie, hellofromTonya, audrasjb, ironprogrammer.
Fixes #57354.
Built from https://develop.svn.wordpress.org/trunk@55008


git-svn-id: https://core.svn.wordpress.org/trunk@54541 1a063a9b-81f0-0310-95a4-ce76da25c4cd
VenusPR added a commit to VenusPR/Wordpress_Richard that referenced this pull request Mar 9, 2023
Adds the ability to define outline CSS properties for elements and blocks within `theme.json` to render `outline-color`, `outline-offset`, `outline-style`, and `outline-width` styles.

Originally developed and tested in [WordPress/gutenberg#43526 Gutenberg PR 43526].

Props onemaggie, hellofromTonya, audrasjb, ironprogrammer.
Fixes #57354.
Built from https://develop.svn.wordpress.org/trunk@55008


git-svn-id: http://core.svn.wordpress.org/trunk@54541 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.
Projects
Status: 🏆 Done
Development

Successfully merging this pull request may close these issues.

Allow styling outline with theme.json
9 participants