UnitControl: Add support for unit step per unit type by rmorse · Pull Request #30376 · WordPress/gutenberg

rmorse · 2021-03-30T10:10:31Z

Description

The UnitControl component does not support a "step" value. Therefor it always increments exactly by 1.

This is probably not desired for certain unit types (i.e., rem, em, etc) in certain situations.

This update, allows for adding a "step" property to the units argument - so it can be controlled on a per unit type basis:

So we can now do this:

    const units = [
    	{ value: 'px', label: 'px', default: 0, step: 1 },
    	{ value: '%', label: '%', default: 10, step: 1 },
    	{ value: 'em', label: 'em', default: 0, step: 0.1 },
    ];

It defaults to a value of 1, if the unit type does not contain a step (for backwards compatibility).

Personally, I wanted to use this as an improved "Line height" control, that also supports units, and the first unit is blank:
{ value: '', label: '', default: 1, step: 0.1 },
But I think there is a good argument for supporting different steps for different use cases on a per unit basis.

How has this been tested?

I've tested the component by following the example, by suppling a step, and without.

All incremements behave as expected.

I have not tested the React Native stuff - but I have updated the code. I checked, and it looks like RangeCell and StepperCell both have a step property so this should work - can someone test this? (I'm not sure how to)

Screenshots

Don't worry about the value reset on unit change - this is not hooked up to state.
This example uses the following props for unit types (notice 0.1 step on em, rem, and empty)

const unitTypes = [
	{ value: '', label: '', default: 1, step: 0.1 },
	{ value: 'px', label: 'px', default: 0 },
	{ value: '%', label: '%', default: 0 },
	{ value: 'em', label: 'em', default: 0, step: 0.1 },
	{ value: 'rem', label: 'rem', default: 0, step: 0.1 },
	{ value: 'vw', label: 'vw', default: 0 },
	{ value: 'vh', label: 'vh', default: 0 },
];

Types of changes

Minor improvement - we check the unit types argument, for a step value and pass that into our number input, if it is not present, default to a step of 1

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

*Update - I guess this also builds on: #31314

rmorse · 2021-03-30T10:14:00Z

packages/components/src/unit-control/index.js

 import UnitSelectControl from './unit-select-control';
 import { CSS_UNITS, getParsedValue, getValidParsedUnit } from './utils';
 import { useControlledState } from '../utils/hooks';
+// import { active } from '@wp-g2/styles/types';


This was causing my local build to fail, and it wasn't in use - so I've commented it for now

Do we still need anything here?

add comments

rmorse · 2021-03-30T16:44:15Z

@ItsJonQ - thought I would flag you on this as you've been working on the previous iterations and will have a sense if this is in the right direction.

paaljoachim · 2021-04-10T11:12:46Z

@aristath @gziolo @mcsf Can you take a look at this PR?
Thanks!

aristath · 2021-04-13T05:27:19Z

What are all the changes to package-lock.json for?

rmorse · 2021-04-13T06:26:29Z

Hmm there shouldn't have been, maybe a bad merge on my side, I'll double check later on today and see if I can get this tidied up.

mcsf · 2021-04-15T11:45:01Z

@rmorse: thanks for the PR. Ensuring that certain units, like em, can default to stepping by 0.1 makes sense as far as the user is concerned. However, I wanted to correct the opening assumption:

The UnitControl component does not support a "step" value. Therefor it always increments exactly by 1.

The component does accept step as a prop, which it will pass down until it reaches NumberControl. I agree that this isn't clear, and the inheritance model used by the *Control components makes it hard to determine what the full API is.

@nosolosw: is the addition of units to line-height compatible with what you're doing in Global Styles?

If so, then @rmorse, we could go with your suggestion to provide step defaults for each unit in CSS_UNITS, as long as we don't override any incoming step prop:

let step = stepProp;
if ( ! step ) {
  const activeUnit = units.find( ( option ) => option.value === unit );
  step = activeUnit?.step ? activeUnit.step : 1;
}

My last note is to treat the non-unit, '', locally in LineHeightControl and not in UnitControl. That's because, in other contexts, it may refer to larger or strictly integer values (e.g. columns: 2).

oandregal · 2021-04-16T13:59:12Z

Things that relate to this from the "theme.json" perspective are:

There's interest in adding units other than pixels for font sizes How to absorb font size units #25137 Add relative units for font size option #23323 so I wonder how/whether these are related to each other.
We do have some units in the theme.json spec https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-json/#settings although they seem to affect only spacing properties (margin, padding), as far as I know. If we add these in the line-height control and others, we may want to add the corresponding setting in theme.json for themes to control which ones to display.

rmorse · 2021-04-18T18:10:34Z

@mcsf - I've updated that. To confirm logic:

The UnitControl can accept a step prop, and that should supercede the step value associated with units
It is expected that this prop, is applied to all units when passed

I've added your suggestion to the PR (and also added a condition to default to the first unit type, if somehow one wasn't found)

mcsf

Thanks for cleaning up. Here's my review.

packages/components/src/unit-control/index.js

mcsf · 2021-04-19T17:16:48Z

packages/components/src/unit-control/index.js

 import UnitSelectControl from './unit-select-control';
 import { CSS_UNITS, getParsedValue, getValidParsedUnit } from './utils';
 import { useControlledState } from '../utils/hooks';
+// import { active } from '@wp-g2/styles/types';


Do we still need anything here?

Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>

remove setting default first unit tidy code + comments

mcsf

Hey, @rmorse, we're almost there! Please apply these last fixes, and we should be good to go.

packages/components/src/unit-control/index.native.js

packages/components/src/unit-control/index.js

packages/components/src/unit-control/index.native.js

Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>

rmorse · 2021-05-11T15:03:03Z

Thanks @mcsf , now done - I didn't understand those fails on units being able to be false - should have spotted the bad variable name though ;)

mcsf

🚢

dcalhoun

LGTM. I tested the changes for native on an iPhone 11 simulator and Samsung Galaxy S20 to test for any regressions.

I also opened wordpress-mobile/gutenberg-mobile#3479 to allow additional native CI tests to run for this change. I'd recommend awaiting those to pass before merging.

dcalhoun · 2021-05-11T17:14:57Z

The current failures in wordpress-mobile/gutenberg-mobile#3479 are unrelated to the changes in this PR and have been resolved in wordpress-mobile/gutenberg-mobile#3477. We should be good to go for merging this work. 👍🏻

mcsf · 2021-05-11T17:17:00Z

Thanks for everyone’s help!

rmorse added 3 commits March 30, 2021 11:31

add support for unit step per unit type

0db17fe

remove default step value from CSS_UNITS const

90476d5

add react native support

9b5c9f7

rmorse requested review from ajitbohra and chrisvanpatten as code owners March 30, 2021 10:10

rmorse commented Mar 30, 2021

View reviewed changes

missed step on native RangeCell

1f9543f

add comments

mcsf added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 15, 2021

rmorse added 2 commits April 18, 2021 19:33

resolve package.json conflict, update repo

815de3d

allow props to override unit steps

48a4997

mcsf reviewed Apr 19, 2021

View reviewed changes

mcsf requested a review from gziolo April 19, 2021 17:17

rmorse and others added 5 commits April 19, 2021 19:25

Update packages/components/src/unit-control/index.js

90b853c

Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>

updates from review ->

f5da8d0

remove setting default first unit tidy code + comments

Merge branch 'trunk' into update/unitcontrol

6a35525

Use capitals in comments

f71082b

Use block commments instead of single line

9d5c673

mcsf requested changes May 10, 2021

View reviewed changes

packages/components/src/unit-control/index.native.js Outdated Show resolved Hide resolved

packages/components/src/unit-control/index.js Outdated Show resolved Hide resolved

packages/components/src/unit-control/index.native.js Outdated Show resolved Hide resolved

rmorse and others added 2 commits May 11, 2021 16:57

Apply suggestions from code review

b0b5a88

Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>

Merge branch 'trunk' into update/unitcontrol

35f6a22

dcalhoun mentioned this pull request May 11, 2021

UnitControl: Add support for unit step per unit type wordpress-mobile/gutenberg-mobile#3479

Closed

2 tasks

mcsf approved these changes May 11, 2021

View reviewed changes

dcalhoun approved these changes May 11, 2021

View reviewed changes

mcsf merged commit 132fec1 into WordPress:trunk May 11, 2021

github-actions bot added this to the Gutenberg 10.7 milestone May 11, 2021

aristath mentioned this pull request May 20, 2021

Spacing Controls Don't Allow Decimals #31964

Closed

aristath mentioned this pull request Jun 15, 2021

Fix: Allow decimals in spacing controls #32692

Merged

7 tasks

Conversation

rmorse commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

rmorse Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsf Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

rmorse commented Mar 30, 2021

Uh oh!

paaljoachim commented Apr 10, 2021

Uh oh!

aristath commented Apr 13, 2021

Uh oh!

rmorse commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcsf commented Apr 15, 2021

Uh oh!

oandregal commented Apr 16, 2021

Uh oh!

rmorse commented Apr 18, 2021

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcsf Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rmorse commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

dcalhoun left a comment

Choose a reason for hiding this comment

Uh oh!

dcalhoun commented May 11, 2021

Uh oh!

mcsf commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rmorse commented Mar 30, 2021 •

edited

Loading

rmorse Mar 30, 2021 •

edited

Loading

rmorse commented Apr 13, 2021 •

edited

Loading

rmorse commented May 11, 2021 •

edited

Loading