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

Move styles.blockgap to settings.spacing.blockGap #34452

Closed
richtabor opened this issue Sep 1, 2021 · 11 comments
Closed

Move styles.blockgap to settings.spacing.blockGap #34452

richtabor opened this issue Sep 1, 2021 · 11 comments
Labels
Needs Decision Needs a decision to be actionable or relevant [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@richtabor
Copy link
Member

richtabor commented Sep 1, 2021

I'm unsure what the logic is behind having blockGap within styles in theme.json, instead of settings.spacing.blockGap.

The value itself is used throughout the editor — instead of applied directory to the body tag, as the other styles that are currently added to styles are. Seems a bit confusing/harder to employ, when spacing configuration aspects of the theme.json file separated as such.

I propose having a true/false boolean as settings.spacing.customBlockGap which would declare whether or not the UI should allow for changing of a block's blockGap value (when blocks have this ability - soon).

And a separate settings.spacing.blockGap entry to indicate what the block gap should be for the theme (say 30px or so) replacing the current styles.blockGap.

How it is today:

Screen Shot 2021-09-01 at 10 03 46 AM

and

Screen Shot 2021-09-01 at 10 05 48 AM

The front-end

The generated CSS property is not actually being applied here, as the other settings are:
Screen Shot 2021-09-01 at 10 00 22 AM

Suggested

Moving the blockGap setting in theme.json to be with the other spacing settings of a theme.

"settings": {
		"spacing": {
                        "blockGap": "30px",
			"customBlockGap": true,
			"customPadding": true,
			"units": [
				"%",
				"px",
				"em",
				"rem",
				"vh",
				"vw"
                ]
},
@richtabor richtabor changed the title Any reason why blockGap is in styles instead of settings -> spacing? Why is blockGap in styles instead of settings -> spacing? Sep 1, 2021
@richtabor richtabor changed the title Why is blockGap in styles instead of settings -> spacing? Why is blockGap in styles instead of settings.spacing.blockGap? Sep 1, 2021
@richtabor richtabor changed the title Why is blockGap in styles instead of settings.spacing.blockGap? Move styles.blockgap to settings.spacing.blockGap Sep 1, 2021
@youknowriad
Copy link
Contributor

The block gap is a style that is actually added to the generate stylesheet. The settings are just config values that are available in the editor.

@youknowriad youknowriad added the [Type] Discussion For issues that are high-level and not yet ready to implement. label Sep 1, 2021
@richtabor
Copy link
Member Author

Isn't the idea that blockGap will be available within the editor — perhaps in Global Styles #32366, but also at the block level in some parent specific #33991 — similar to color palettes?

@youknowriad
Copy link
Contributor

#33991 is a PR that allows you to define a specific block style for a given block. (so still a style).

And now adding block gap to body actually prints the CSS variable in the generate style based on theme.json (just like all other styles).

The settings.spacing.blockGap is a setting that is introduced in #33991 to allow the UI to tweak the spacing per container block to be visible or not (just like other settings customColor, gradients, fontSize...)

@richtabor
Copy link
Member Author

Thanks for the clarity there, but why wouldn't #33991 be "customBlockGap": true — like the others?

@youknowriad
Copy link
Contributor

I think that's good feedback, you could ask there. I believe it started as "customBlockGap" and was renamed. @andrewserong

@fwazeter
Copy link
Member

fwazeter commented Sep 1, 2021

for naming convention, I think it'd be best to keep things in alignment with whatever standard that ends up being - whether we go more verbose with customSettingName (customBlockGap) or settingName (blockGap), and the location placements to be consistent where other related elements are in place, otherwise it becomes a nightmare to dig through documentation on where you messed up a setting placement.

@youknowriad
Copy link
Contributor

One small clarification is that customColor and customGradient.... make sense because these mean: folks are allowed to use "custom colors" or not. and "colors", "gradients" are about allowing and defining preset colors and gradients. In Blog Gap, there's no difference there, there's only custom block gap, there's no presets (at least at the moment, and I'm not sure if we'd have presets)

@richtabor
Copy link
Member Author

In this model, by customBlockGap being false, would that remove the UI to modify blockGap values on a per-block basis? Say change the blockGap between columns in the Columns block?

@youknowriad
Copy link
Contributor

In this model, by customBlockGap being false, would that remove the UI to modify blockGap values on a per-block basis? Say change the blockGap between columns in the Columns block?

Right now, it's still named just blockGap in the PR but yes it would (since we don't have presets, so custom values are the only way to define gaps per block)

@fwazeter
Copy link
Member

fwazeter commented Sep 1, 2021

One small clarification is that customColor and customGradient.... make sense because these mean: folks are allowed to use "custom colors" or not. and "colors", "gradients" are about allowing and defining preset colors and gradients. In Blog Gap, there's no difference there, there's only custom block gap, there's no presets (at least at the moment, and I'm not sure if we'd have presets)

I'd largely agree that customColor and customGradient makes sense because that explains explicitly the setting enabled.

@andrewserong
Copy link
Contributor

I think that's good feedback, you could ask there. I believe it started as "customBlockGap" and was renamed.

Thanks for the ping (and for starting the discussion, Rich, much appreciated)! Yes, the setting was renamed for consistency with the plan proposed in the theme.json v2 issue that suggests removing the custom prefix from those spacing settings where the word isn't needed. My goal is to try to keep #33991 consistent with the direction we're heading in, so that we don't have to later rename it.

For where the setting versus styling sits, I think it's worth keeping them separate between styles and settings for consistency with the padding and margin settings and values. When specifying the blockGap for an individual block, I imagine we'll often want to tweak the value to be consistent with a padding or margin value, so it'd be handy for that to all happen in the same part of the JSON. For example, if we think of the Columns block, we might wind up with something that looks like the following:

	"styles": {
		...
		"blocks": {
			...
			"core/columns": {
				"spacing": {
					"blockGap": "2em",
					"padding": "2em"    
				}
			},
		}
	}

Very happy for feedback on that, though, since I'm likely biased from tinkering with #33991 ! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision Needs a decision to be actionable or relevant [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
None yet
Development

No branches or pull requests

5 participants