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

Units block support in block.json does not work #47519

Open
carolinan opened this issue Jan 28, 2023 · 11 comments
Open

Units block support in block.json does not work #47519

carolinan opened this issue Jan 28, 2023 · 11 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.

Comments

@carolinan
Copy link
Contributor

Description

Three core blocks currently has a block support in block.json called units: gallery, navigation, and social links.
The purpose of this block support is undocumented, and it is not in the schema for block.json.

Assuming that the intention was to set the default units that the block uses in different controls, the support is not working.
It is possible that this was working when it was implemented, but has regressed.

For example the navigation block declares:

"spacing": {
            "blockGap": true,
            "units": [ "px", "em", "rem", "vh", "vw" ],
            "__experimentalDefaultControls": {
                "blockGap": true
            }
        },

But the block spacing control has the units px, em and rem.

In the gallery block, units are directly under supports, not under spacing.

	"supports": {
		...
		"units": [ "px", "em", "rem", "vh", "vw" ],

But only the spacing control uses units, and they are px, em and rem.

If I update these blocks to for example only use the px unit, there is no change in the editor controls.
If spacing units are also added in theme.json, then the theme.json values are used.

Step-by-step reproduction instructions

  1. Activate a theme that does not add spacing units, for example emptytheme.
  2. Add a gallery block.
  3. In the Dimensions panel, block spacing, select "Set custom size".
  4. In the control, select the button with the text "px". This opens the unit options. Confirm that only px, rem and em are available.

Screenshots, screen recording, code snippet

No response

Environment info

WP 6.2-alpha-55147
Gutenberg trunk

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@carolinan carolinan added [Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended [Type] Question Questions about the design or development of the editor. and removed [Type] Question Questions about the design or development of the editor. labels Jan 28, 2023
@ndiego ndiego moved this to ❓ Triage in WordPress 6.2 Editor Tasks Jan 30, 2023
@ndiego ndiego moved this from ❓ Triage to 🦵 Punted to 6.3 in WordPress 6.2 Editor Tasks Feb 9, 2023
@gziolo gziolo added the Needs Dev Ready for, and needs developer efforts label Feb 27, 2023
@ndiego ndiego moved this to ❓ Triage in WordPress 6.3.x Editor Tasks May 23, 2023
@skorasaurus
Copy link
Member

skorasaurus commented Dec 1, 2023

I'm unable to reproduce this in Gutenberg 17 and 6.4; I can use vh and vw in the gallery block as indicated below.

However, I wonder if we should leave this open because I do notice that the gallery block's support has supports.units

but the navigation block has supports.spacing.units

Is that intentional?

my theme.json 
      "spacing":{
         "padding":true,
         "margin":true,
         "units":[
            "px",
            "em",
            "rem",
            "vh",
            "vw",
            "ch"
         ]
      }

For example:

image

image

@skorasaurus skorasaurus added Needs Technical Feedback Needs testing from a developer perspective. and removed Needs Dev Ready for, and needs developer efforts labels Dec 1, 2023
@carolinan
Copy link
Contributor Author

theme.json values work, but block.json does not.

@t-hamano
Copy link
Contributor

t-hamano commented Dec 5, 2023

I've tested it in all major versions since WP6.0, when the gallery block first supported gap. Overriding units in block.json does not originally work, and it does not appear to be a bug or regression. Therefore, I will label this issue as Enhancement.

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended Needs Technical Feedback Needs testing from a developer perspective. labels Dec 5, 2023
@carolinan
Copy link
Contributor Author

Perhaps @glendaviesnz or @ramonjd knows the purpose of these block supports or if they can even be removed from the block.json in the respective blocks.

@ramonjd
Copy link
Member

ramonjd commented Jan 3, 2024

Thanks for the ping @carolinan, and for testing @t-hamano

I can confirm @carolinan's observation: setting spacing.units in block.json has no effect. Only in theme.json can I set units that affect UI controls:

		"spacing": {
			"units": true // default is [ "px", "em", "rem", "vh", "vw", "%" ]
		}

Border units are also taken from spacing and therefore controlled by theme.json.

And the unit control component has the same default (uses spacing units):

availableUnits: availableUnits || [ '%', 'px', 'em', 'rem', 'vw' ],

I'd lean on @glendaviesnz's knowledge in relation to spacing units.

As for others, typography font size units are hard-coded it appears:

availableUnits: unitsProp || [ 'px', 'em', 'rem' ],

To remain consistent with other settings, should block.json units override theme.json settings?

Looking at the code, I suppose it would be possible to grab the block.json overrides where it makes sense:

        // For units that use spacing.units
	const [ availableUnits ] = useSettings( 'spacing.units' ); // theme.json
        const blockUnits = getBlockSupport( blockName, 'spacing.units' ); // block.json
	const units = useCustomUnits( {
		availableUnits: availableUnits || blockUnits || [ 'px', 'em', 'rem' ],
	} );

Or maybe the better place would be useSettingsForBlockElement, which states to be a "React hook that overrides a global settings object with block and element specific settings."

We'd probably want to audit the places we use custom units to ensure values make sense, e.g., would vh and vw be appropriate for font sizes? We could just leave it up to block consumers to decide as well.

@carolinan
Copy link
Contributor Author

Shouldn't block.json provide defaults, and theme.json override them?

@ramonjd
Copy link
Member

ramonjd commented Jan 4, 2024

To remain consistent with other settings, should block.json units override theme.json settings?

Sorry, that came out completely wrong. 😄 🙃

I meant block.json supports settings could be the place where a block defines whether it supports something, e.g., units. And you're right, theme.json overrides values of supported settings.

@t-hamano
Copy link
Contributor

t-hamano commented Jan 4, 2024

I'm a bit confused, is the unit property something that should be defined in block.json ? In other words, defining units is entirely the responsibility of theme.json, and core blocks might not need to provide default units via block.json ?

For example, if we want to change only the unit of a specific block, we can define it in theme.json as follows. I have confirmed that this definition actually works as expected.

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"spacing": {
			"units": [ "px", "%" ]
		},
		"blocks": {
			"core/group": {
				"spacing": {
					"units": [ "vh", "vw" ]
				}
			}
		}
	}
}

If this assumption is correct, is the unit defined in block.json some kind of mistake and should we just delete it?

@ramonjd
Copy link
Member

ramonjd commented Jan 4, 2024

I'm a bit confused, is the unit property something that should be defined in block.json ? In other words, defining units is entirely the responsibility of theme.json, and core blocks might not need to provide default units via block.json ?

It's a good question.

I think a blocks' block.json could still define defaults if it makes sense in context. Maybe the % or vh might not make good defaults for some block types, e.g., text blocks. Now, all this assumes that the functionality to make this work will be built 😄

Another example, I suspect the gallery block defined "units": [ "px", "em", "rem", "vh", "vw" ], (even though it has no effect) to provide specific defaults for block gap in #38164

@aaronrobertshaw will also have some insight into supports.

@aaronrobertshaw
Copy link
Contributor

I think a blocks' block.json could still define defaults if it makes sense in context. Maybe the % or vh might not make good defaults for some block types, e.g., text blocks.

My take on this is the block.json might define more the "available" units rather than defaults. So as you say, the block could cull units that do not make sense for it at all. Theme.json can then still define which units are actually used or shown.

#31822 might provide some further context around units.

Another example, I suspect the gallery block defined "units": [ "px", "em", "rem", "vh", "vw" ], (even though it has no effect) to provide specific defaults for block gap in #38164

At a quick glance of that PR and some of the other gallery code at the time of that commit, I couldn't see where the units defined in block.json were actually used. @glendaviesnz might have some extra details buried deep within his memory 🤞

@carolinan
Copy link
Contributor Author

This is also relevant if we want to add units that are specific to font sizes.
@t-hamano

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

6 participants