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

NumberControl: Add TypeScript prop types #43791

Merged
merged 21 commits into from
Sep 8, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export const InputWithSlider = ( {
label={ label }
hideLabelFromVision
value={ value }
// @ts-expect-error TODO: Resolve discrepancy in NumberControl
onChange={ onChange }
prefix={
<Spacer
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/input-control/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ export interface InputBaseProps extends BaseProps, FlexProps {
* @default false
*/
disabled?: boolean;
/**
* The class name to be added to the wrapper element.
*/
className?: string;
id?: string;
/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// @ts-nocheck
/**
* External dependencies
*/
import classNames from 'classnames';
import type { ForwardedRef } from 'react';

/**
* WordPress dependencies
Expand All @@ -17,8 +17,11 @@ import { Input } from './styles/number-control-styles';
import * as inputControlActionTypes from '../input-control/reducer/actions';
import { add, subtract, roundClamp } from '../utils/math';
import { isValueEmpty } from '../utils/values';
import type { WordPressComponentProps } from '../ui/context/wordpress-component';
import type { NumberControlProps } from './types';
import type { InputState } from '../input-control/reducer/state';

export function NumberControl(
function UnforwardedNumberControl(
{
__unstableStateReducer: stateReducerProp,
className,
Expand All @@ -35,32 +38,36 @@ export function NumberControl(
type: typeProp = 'number',
value: valueProp,
...props
},
ref
}: WordPressComponentProps< NumberControlProps, 'input', false >,
ref: ForwardedRef< any >
) {
const isStepAny = step === 'any';
// @ts-expect-error step should be a number but could be string
ciampo marked this conversation as resolved.
Show resolved Hide resolved
const baseStep = isStepAny ? 1 : parseFloat( step );
const baseValue = roundClamp( 0, min, max, baseStep );
const constrainValue = ( value, stepOverride ) => {
const constrainValue = ( value: number, stepOverride?: number | null ) => {
ciampo marked this conversation as resolved.
Show resolved Hide resolved
// When step is "any" clamp the value, otherwise round and clamp it.
return isStepAny
? Math.min( max, Math.max( min, value ) )
: roundClamp( value, min, max, stepOverride ?? baseStep );
};

const autoComplete = typeProp === 'number' ? 'off' : null;
const autoComplete = typeProp === 'number' ? 'off' : undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Runtime change: null is an invalid value for the autoComplete attribute.

const classes = classNames( 'components-number-control', className );

/**
* "Middleware" function that intercepts updates from InputControl.
* This allows us to tap into actions to transform the (next) state for
* InputControl.
*
* @param {Object} state State from InputControl
* @param {Object} action Action triggering state change
* @return {Object} The updated state to apply to InputControl
* @return The updated state to apply to InputControl
*/
const numberControlStateReducer = ( state, action ) => {
const numberControlStateReducer = (
/** State from InputControl. */
state: InputState,
/** Action triggering state change. */
action: inputControlActionTypes.InputAction
) => {
mirka marked this conversation as resolved.
Show resolved Hide resolved
const nextState = { ...state };

const { type, payload } = action;
Expand All @@ -74,10 +81,12 @@ export function NumberControl(
type === inputControlActionTypes.PRESS_UP ||
type === inputControlActionTypes.PRESS_DOWN
) {
// @ts-expect-error TODO: Investigate if this is wrong
mirka marked this conversation as resolved.
Show resolved Hide resolved
const enableShift = event.shiftKey && isShiftStepEnabled;

const incrementalValue = enableShift
? parseFloat( shiftStep ) * baseStep
? // @ts-expect-error shiftStep should be a number but could be string
parseFloat( shiftStep ) * baseStep
: baseStep;
let nextValue = isValueEmpty( currentValue )
? baseValue
Expand All @@ -88,14 +97,18 @@ export function NumberControl(
}

if ( type === inputControlActionTypes.PRESS_UP ) {
// @ts-expect-error TODO: Investigate if this is wrong
nextValue = add( nextValue, incrementalValue );
}

if ( type === inputControlActionTypes.PRESS_DOWN ) {
// @ts-expect-error TODO: Investigate if this is wrong
nextValue = subtract( nextValue, incrementalValue );
}

// @ts-expect-error TODO: Investigate if this is wrong
nextState.value = constrainValue(
// @ts-expect-error TODO: Investigate if this is wrong
ciampo marked this conversation as resolved.
Show resolved Hide resolved
nextValue,
enableShift ? incrementalValue : null
);
Expand All @@ -105,10 +118,13 @@ export function NumberControl(
* Handles drag to update events
*/
if ( type === inputControlActionTypes.DRAG && isDragEnabled ) {
// @ts-expect-error TODO: Investigate
const [ x, y ] = payload.delta;
// @ts-expect-error TODO: Investigate
const enableShift = payload.shiftKey && isShiftStepEnabled;
mirka marked this conversation as resolved.
Show resolved Hide resolved
const modifier = enableShift
? parseFloat( shiftStep ) * baseStep
? // @ts-expect-error shiftStep should be a number but could be string
parseFloat( shiftStep ) * baseStep
: baseStep;

let directionModifier;
Expand Down Expand Up @@ -140,7 +156,9 @@ export function NumberControl(
delta = Math.ceil( Math.abs( delta ) ) * Math.sign( delta );
const distance = delta * modifier * directionModifier;

// @ts-expect-error TODO: Investigate if this is wrong
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "Investigate if this is wrong", should the comment be more about the fact that this error will only be solved once we make a decision on the value type?

(same for the rest of the @ts-expect-error comments about the type of the value prop)

Copy link
Member Author

@mirka mirka Sep 7, 2022

Choose a reason for hiding this comment

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

Good call, I improved the TODO comments all around. 90948f0

One of the interesting causes was the isValueEmpty() util function. I was able to fix the typing, but it's a bit out of scope for this PR so I'll submit it separately.

nextState.value = constrainValue(
// @ts-expect-error TODO: Investigate if this is wrong
add( currentValue, distance ),
enableShift ? modifier : null
);
Expand All @@ -156,9 +174,11 @@ export function NumberControl(
) {
const applyEmptyValue = required === false && currentValue === '';

// @ts-expect-error TODO: Investigate if this is wrong
nextState.value = applyEmptyValue
? currentValue
: constrainValue( currentValue );
: // @ts-expect-error TODO: Investigate if this is wrong
constrainValue( currentValue );
}

return nextState;
Expand All @@ -180,6 +200,7 @@ export function NumberControl(
required={ required }
step={ step }
type={ typeProp }
// @ts-expect-error TODO: Resolve discrepancy
value={ valueProp }
__unstableStateReducer={ ( state, action ) => {
const baseState = numberControlStateReducer( state, action );
Expand All @@ -189,4 +210,6 @@ export function NumberControl(
);
}

export default forwardRef( NumberControl );
export const NumberControl = forwardRef( UnforwardedNumberControl );

export default NumberControl;
22 changes: 4 additions & 18 deletions packages/components/src/number-control/stories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,10 @@ export default {
title: 'Components (Experimental)/NumberControl',
component: NumberControl,
argTypes: {
size: {
control: {
type: 'select',
options: [ 'default', 'small', '__unstable-large' ],
},
},
onChange: { action: 'onChange' },
prefix: { control: { type: 'text' } },
suffix: { control: { type: 'text' } },
type: { control: { type: 'text' } },
mirka marked this conversation as resolved.
Show resolved Hide resolved
},
};

Expand All @@ -44,16 +41,5 @@ function Template( { onChange, ...props } ) {

export const Default = Template.bind( {} );
Default.args = {
disabled: false,
hideLabelFromVision: false,
isPressEnterToChange: false,
isShiftStepEnabled: true,
label: 'Number',
min: 0,
max: 100,
placeholder: '0',
required: false,
shiftStep: 10,
size: 'default',
step: '1',
label: 'Value',
};
80 changes: 80 additions & 0 deletions packages/components/src/number-control/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* External dependencies
*/
import type { HTMLInputTypeAttribute, InputHTMLAttributes } from 'react';

/**
* Internal dependencies
*/
import type { InputControlProps } from '../input-control/types';

export type NumberControlProps = Omit<
InputControlProps,
'isDragEnabled' | 'min' | 'max' | 'required' | 'type' | 'value'
mirka marked this conversation as resolved.
Show resolved Hide resolved
> & {
/**
* If true, the default `input` HTML arrows will be hidden.
*
* @default false
*/
hideHTMLArrows?: boolean;
/**
* If true, enables mouse drag gestures.
*
* @default true
*/
isDragEnabled?: boolean;
/**
* If true, pressing `UP` or `DOWN` along with the `SHIFT` key will increment the
* value by the `shiftStep` value.
*
* @default true
*/
isShiftStepEnabled?: boolean;
/**
* The maximum `value` allowed.
*
* @default Infinity
*/
max?: number;
/**
* The minimum `value` allowed.
*
* @default -Infinity
*/
min?: number;
/**
* If `true` enforces a valid number within the control's min/max range.
* If `false` allows an empty string as a valid value.
*
* @default false
*/
required?: boolean;
/**
* Amount to increment by when the `SHIFT` key is held down. This shift value is
* a multiplier to the `step` value. For example, if the `step` value is `5`,
* and `shiftStep` is `10`, each jump would increment/decrement by `50`.
*
* @default 10
*/
shiftStep?: number;
/**
* Amount by which the `value` is changed when incrementing/decrementing.
* It is also a factor in validation as `value` must be a multiple of `step`
* (offset by `min`, if specified) to be valid. Accepts the special string value `any`
* that voids the validation constraint and causes stepping actions to increment/decrement by `1`.
*
* @default 1
*/
step?: InputHTMLAttributes< HTMLInputElement >[ 'step' ] | 'any';
ciampo marked this conversation as resolved.
Show resolved Hide resolved
/**
* The `type` attribute of the `input` element.
*
* @default 'number'
*/
type?: HTMLInputTypeAttribute;
/**
* The value of the input.
*/
value?: number | string;
};
4 changes: 3 additions & 1 deletion packages/components/src/range-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ function UnforwardedRangeControl< IconProps = unknown >(
onChange( nextValue );
};

const handleOnChange = ( next: string ) => {
const handleOnChange = ( next?: string ) => {
// @ts-expect-error TODO: Investigate if this is problematic
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use ensureNumber ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one can't be fixed by ensureNumber because the problem is that next can potentially be undefined.

Technically, we can remove the ts-ignore without changing effective runtime behavior by doing something like this:

let nextValue = typeof next === 'undefined' ? NaN : parseFloat( next );

But then again, I really think this one needs further investigation because it's unclear whether it's intended for setValue() to sometimes receive a NaN. It could actually be problematic, so I wouldn't want to imply that this behavior is legit.

let nextValue = parseFloat( next );
setValue( nextValue );

Expand Down Expand Up @@ -304,6 +305,7 @@ function UnforwardedRangeControl< IconProps = unknown >(
onChange={ handleOnChange }
shiftStep={ shiftStep }
step={ step }
// @ts-expect-error TODO: Investigate if the `null` value is necessary
value={ inputSliderValue }
/>
) }
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/unit-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,6 @@ function UnforwardedUnitControl(
return (
<Root className="components-unit-control-wrapper" style={ style }>
<ValueInput
aria-label={ label }
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Runtime change: This label variable is a ReactNode, but aria-label only accepts strings. I went ahead and removed this redundant aria-label, because the same value is added on line 270 via label prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noted. Basically the input will be labelled via an actual label element (with the for attribute), instead of the aria-label attribute 👌

type={ isPressEnterToChange ? 'text' : 'number' }
{ ...props }
autoComplete={ autoComplete }
Expand Down
Loading