New NumberControl component #13863

m-e-h · 2019-02-13T16:34:49Z

Description

Adds a <NumberControl> component for use in other components and blocks.

Would reduce duplicate code and create a consistent style across existing components that are creating their own number inputs. Currently, this would be font-size-picker, focal-point-picker, and range-control that I know of. Probably others.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

Inspired by #13709 and #11555

…spinners." This reverts commit d26c659.

m-e-h · 2019-02-13T16:37:34Z

Not sure if I should update existing components in the same PR or if they should be done individually after the NumberControl component is merged.
I've added them here to make it easier to test but they can be removed.

jorgefilipecosta

Thank you for your contribution @m-e-h, I think it may make sense to expose a very simple component that just renders a number input field.
Using it as the basis for other components may make the work of creating reactive native components simpler as it reduces direct dom usage.
But I would like more thoughts about creating a new component cc: @youknowriad

jorgefilipecosta · 2019-02-13T18:22:38Z

packages/components/src/focal-point-picker/index.js

-							value={ this.fractionToPercentage( percentages.x ) }
-						/>
+
+					<NumberControl


It looks like here we have a NumberControl inside a BaseControl. NumberControl renders its own BaseControl component so I think the existing BaseControl can also be removed.

Thanks for looking @jorgefilipecosta! I'm open to any tips or ideas here. I'm pretty new to the components code.

Yeah, we could do away with the BaseControl around each input here with the focal-point-picker. Will have to restyle though to handle the <span>%</span> appended to each.

Might even be worth adding a numericUnit prop in the NumberControl to handle it.

packages/components/src/font-size-picker/index.js

chrisvanpatten · 2019-02-13T19:27:40Z

packages/components/src/number-control/style.scss

+	display: inline-block;
+	font-weight: 500;
+	height: 30px;
+	width: 54px;


Love the idea of a dedicated NumberControl but nervous about defining the width here. This makes it tougher to use as a full-width control. Maybe there could be a modifier class (.components-number-control__input--narrow or something like that) which applies the width constraint?

youknowriad · 2019-02-14T07:27:02Z

Noting that the TextComponent supports the type prop which means this new component is somehow a duplicate?

m-e-h · 2019-02-14T16:49:17Z

Whoa. I hadn't even thought of that @youknowriad.

I think there are enough blocks ignoring that and using their own number input that it warrants it's own component. Better to repeat the TextControl once as a new component than to repeat similar code each time a number input is used.

Wondering though if a NumberControl component should just import the TextControl and change the type? Then it's not really a "duplicate", it would just be a more obvious choice for devs that need it.

chrisvanpatten · 2019-02-14T18:25:09Z

I like the idea of a dedicated NumberControl because it could allow for additional number functionality in the future (perhaps including custom buttons to increment/decrement, or basic validation, or something along those lines), but I agree; it should pull from the TextControl as a base at least until other UI needs become visible.

m-e-h · 2019-02-14T21:58:35Z

One challenge I ran into when trying to use the TextControl for this, is that it's defining onChange in the return rather than in the defautProps.
This makes it readonly for things like the Focal Point Picker that define their own onChange={ this.horizontalPositionChanged }.

It's likely that I'm missing something. Is there a different way I should be doing it?

youknowriad · 2019-02-15T13:09:51Z

One challenge I ran into when trying to use the TextControl for this, is that it's defining onChange in the return rather than in the defautProps.

That doesn't change anything for me. If the value is not changing, it simply means the value prop is not updated properly for me.

afercia · 2019-02-15T13:53:12Z

A generic component shouldn't assume an aria-label is always desired. Input fields need to be labelled, In order of preference:

a visible label element associated with for/id attributes: in the BaseControl component this is controlled by the presence of both label && id props
if there's no id, then the label value is rendered as plain text and the input is supposed to be labelled with an 'aria-label`

Never do both together.

Some controls in the codebase are misleading, for example the FocalPointPicker renders two input fields that are completely unlabelled. The visible text is just text. There's no <label> element or aria-label... going to create an issue.

m-e-h · 2019-02-20T20:39:44Z

I updated this to better handle id and aria-label per @afercia

I'm sticking with wrapping this in it's own BaseControl. Pulling in the TextControl wasn't matching up the for=/id attributes and I struggled to find an elegant way to get at the BaseControl props.

I also added an optional numberUnit prop so a unit of measurement could be added, like the % on the focal-point component.

I also updated the spacer block to see how the NumberControl worked there.

jorgefilipecosta

Hi @m-e-h,
Thank you for iterating on this PR.
I did a set of tests, and this PR seems almost ready, I just left some minor suggestions.

Besides the suggestions, I think this PR needs a "New Features" changelog entry in https://github.com/WordPress/gutenberg/blob/master/packages/components/CHANGELOG.md#720-unreleased, and a rebase.

jorgefilipecosta · 2019-03-18T09:08:48Z

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

+				{ ...props }
+			/>
+
+			{ numberUnit && (


Maybe we can update the Number control, to pass children to base control. It would allow us to remove the need for a numbeUnit prop making the component simpler. Current usages would only need to pass span prop with the unit like they do now.

jorgefilipecosta · 2019-03-18T09:24:32Z

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

@@ -89,10 +89,9 @@ function RangeControl( {
 				max={ max }
 				{ ...props } />
 			{ afterIcon && <Dashicon icon={ afterIcon } /> }
-			<input
+			<NumberControl


Range control is a complex control in itself with validation logic that was recently added. The component already uses a BaseControl so using a NumberControl here would result in a BaseControl nested inside another BaseControl. I would prefer if we did not use NumberControl inside RangeControl at least for now.

jorgefilipecosta · 2019-03-18T09:39:28Z

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

+	numberUnit,
+	...props
+} ) {
+	const onChangeValue = ( event ) => onChange( Number( event.target.value ) );


Maybe we can change this line to:

const onChangeValue = ( event ) => onChange( event.target.value === '' ? undefined : Number( event.target.value ) );

The current version causes a bug -- it becomes impossible to have the field empty. It is possible to reproduce the bug testing the spacer.
The bug happens because Number('') === 0;

ZebulanStanphill · 2019-11-30T19:58:49Z

What is the status of this PR?

m-e-h · 2019-12-01T16:39:43Z

This can probably be closed. The goal here was mostly to clean up the UI style variations across the different number inputs.
There are probably simpler ways to do that.

m-e-h added 10 commits February 5, 2019 17:44

wider range number controls to account for OS specific input spinners.

d26c659

Revert "wider range number controls to account for OS specific input …

08bfa3c

…spinners." This reverts commit d26c659.

Merge remote-tracking branch 'upstream/master'

3a96234

Merge remote-tracking branch 'upstream/master'

81c9a29

Merge remote-tracking branch 'upstream/master'

a1b4c70

Merge remote-tracking branch 'upstream/master'

acc8d53

New Component: NumberControl

0a1a0bd

Use NumberControl in Focal Point Picker

50d55d7

Use NumberControl in Font Size Picker

91faf95

Use NumberControl in RangeControl

527c2b8

m-e-h requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners February 13, 2019 16:34

jorgefilipecosta reviewed Feb 13, 2019

View reviewed changes

m-e-h added 2 commits February 13, 2019 13:44

lint

7a70962

margin-left only for range component

9f77371

chrisvanpatten reviewed Feb 13, 2019

View reviewed changes

m-e-h added 2 commits February 13, 2019 15:57

update range test

fca7bbe

fix typo

4950d49

gziolo added this to the 5.2 (Gutenberg) milestone Feb 13, 2019

gziolo added [Feature] UI Components Impacts or related to the UI component system [Type] Task Issues or PRs that have been broken down into an individual action to take labels Feb 13, 2019

m-e-h mentioned this pull request Feb 15, 2019

Block-specific responsive controls #13363

Open

m-e-h mentioned this pull request Feb 19, 2019

Match height of font range control with the size select. #11555

Merged

4 tasks

m-e-h added 5 commits February 19, 2019 15:26

Merge remote-tracking branch 'upstream/master'

32b8e7b

Merge remote-tracking branch 'upstream/master'

23124d7

Merge branch 'master' into add/component-number-control

af2c259

Make id and area-label optional

ad87c99

use NumberControl in the spacer block

fa608ee

m-e-h requested review from notnownikki and Soean as code owners February 20, 2019 20:20

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

jorgefilipecosta reviewed Mar 18, 2019

View reviewed changes

jorgefilipecosta added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 20, 2019

ZebulanStanphill closed this Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New NumberControl component #13863

New NumberControl component #13863

m-e-h commented Feb 13, 2019

m-e-h commented Feb 13, 2019

jorgefilipecosta left a comment

jorgefilipecosta Feb 13, 2019

m-e-h Feb 13, 2019

chrisvanpatten Feb 13, 2019

youknowriad commented Feb 14, 2019

m-e-h commented Feb 14, 2019

chrisvanpatten commented Feb 14, 2019

m-e-h commented Feb 14, 2019

youknowriad commented Feb 15, 2019

afercia commented Feb 15, 2019

m-e-h commented Feb 20, 2019

jorgefilipecosta left a comment

jorgefilipecosta Mar 18, 2019

jorgefilipecosta Mar 18, 2019

jorgefilipecosta Mar 18, 2019

ZebulanStanphill commented Nov 30, 2019

m-e-h commented Dec 1, 2019

New NumberControl component #13863

New NumberControl component #13863

Conversation

m-e-h commented Feb 13, 2019

Description

Types of changes

Checklist:

m-e-h commented Feb 13, 2019

jorgefilipecosta left a comment

Choose a reason for hiding this comment

jorgefilipecosta Feb 13, 2019

Choose a reason for hiding this comment

m-e-h Feb 13, 2019

Choose a reason for hiding this comment

chrisvanpatten Feb 13, 2019

Choose a reason for hiding this comment

youknowriad commented Feb 14, 2019

m-e-h commented Feb 14, 2019

chrisvanpatten commented Feb 14, 2019

m-e-h commented Feb 14, 2019

youknowriad commented Feb 15, 2019

afercia commented Feb 15, 2019

m-e-h commented Feb 20, 2019

jorgefilipecosta left a comment

Choose a reason for hiding this comment

jorgefilipecosta Mar 18, 2019

Choose a reason for hiding this comment

jorgefilipecosta Mar 18, 2019

Choose a reason for hiding this comment

jorgefilipecosta Mar 18, 2019

Choose a reason for hiding this comment

ZebulanStanphill commented Nov 30, 2019

m-e-h commented Dec 1, 2019