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.

Sign up for GitHub

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

UnitControl component: Refactor utils to TypeScript #35138

Merged
merged 7 commits into from
Sep 29, 2021
Merged

UnitControl component: Refactor utils to TypeScript #35138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f21349c
UnitControl: Refactor utils to TypeScript
andrewserong Sep 27, 2021
c4f129a
Update return value doc comment for getValidParsedUnit
andrewserong Sep 28, 2021
11e7f22
Adjust types, add story for custom units
andrewserong Sep 28, 2021
fa9fe44
Use Value in more places for consistency
andrewserong Sep 28, 2021
8d4976a
Update default attribute on WPUnitControlUnit to use Value type
andrewserong Sep 28, 2021
224d868
Add WPUnitControlUnitList type
andrewserong Sep 29, 2021
b7030b3
Add contextual comment to the Number.isFinite check
andrewserong Sep 29, 2021
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
47 changes: 44 additions & 3 deletions packages/components/src/unit-control/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ export default {
component: UnitControl,
};

const ControlWrapperView = styled.div`
max-width: 80px;
`;

function Example() {
const [ value, setValue ] = useState( '10px' );

Expand Down Expand Up @@ -60,6 +64,43 @@ export const _default = () => {
return <Example />;
};

const ControlWrapperView = styled.div`
max-width: 80px;
`;
export function WithCustomUnits() {
const [ value, setValue ] = useState( '10km' );

const props = {
isResetValueOnUnitChange: boolean( 'isResetValueOnUnitChange', true ),
label: text( 'label', 'Distance' ),
units: object( 'units', [
{
value: 'km',
label: 'km',
default: 1,
},
{
value: 'mi',
label: 'mi',
default: 1,
},
{
value: 'm',
label: 'm',
default: 1000,
},
{
value: 'yd',
label: 'yd',
default: 1760,
},
] ),
};

return (
<ControlWrapperView>
<UnitControl
{ ...props }
value={ value }
onChange={ ( v ) => setValue( v ) }
/>
</ControlWrapperView>
);
}
9 changes: 9 additions & 0 deletions packages/components/src/unit-control/test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,15 @@ describe( 'UnitControl utils', () => {
filterUnitsWithSettings( preferredUnits, availableUnits )
).toEqual( [] );
} );

it( 'should return empty array where available units is set to false', () => {
const preferredUnits = [ '%', 'px' ];
const availableUnits = false;

expect(
filterUnitsWithSettings( preferredUnits, availableUnits )
).toEqual( [] );
} );
} );

describe( 'getValidParsedUnit', () => {
Expand Down
26 changes: 26 additions & 0 deletions packages/components/src/unit-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
export type Value = number | string;

export type WPUnitControlUnit = {
/**
* The value for the unit, used in a CSS value (e.g `px`).
*/
value: string;
ciampo marked this conversation as resolved.
Show resolved Hide resolved
/**
* The label used in a dropdown selector for the unit.
*/
label: string;
/**
* Default value for the unit, used when switching units.
*/
default?: Value;
/**
* An accessible label used by screen readers.
*/
a11yLabel?: string;
/**
* A step value used when incrementing/decrementing the value.
*/
step?: number;
};

export type WPUnitControlUnitList = Array< WPUnitControlUnit > | false;
141 changes: 78 additions & 63 deletions ...ages/components/src/unit-control/utils.js → ...ages/components/src/unit-control/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,17 @@
/**
* External dependencies
*/
import { isEmpty } from 'lodash';
andrewserong marked this conversation as resolved.
Show resolved Hide resolved

/**
* WordPress dependencies
*/
import { __, _x } from '@wordpress/i18n';
import { Platform } from '@wordpress/element';

/**
* An object containing the details of a unit.
*
* @typedef {Object} WPUnitControlUnit
* @property {string} value The value for the unit, used in a CSS value (e.g `px`).
* @property {string} label The label used in a dropdown selector for the unit.
* @property {string|number} [default] Default value for the unit, used when switching units.
* @property {string} [a11yLabel] An accessible label used by screen readers.
* @property {number} [step] A step value used when incrementing/decrementing the value.
* Internal dependencies
*/
import type { Value, WPUnitControlUnit, WPUnitControlUnitList } from './types';

const isWeb = Platform.OS === 'web';

/** @type {Record<string, WPUnitControlUnit>} */
const allUnits = {
const allUnits: Record< string, WPUnitControlUnit > = {
px: {
value: 'px',
label: isWeb ? 'px' : __( 'Pixels (px)' ),
Expand Down Expand Up @@ -157,12 +145,16 @@ export const DEFAULT_UNIT = allUnits.px;
* Moving forward, ideally the value should be a string that contains both
* the value and unit, example: '10px'
*
* @param {number|string} value Value
* @param {string} unit Unit value
* @param {Array<WPUnitControlUnit>} units Units to derive from.
* @return {Array<number, string>} The extracted number and unit.
* @param value Value
* @param unit Unit value
* @param units Units to derive from.
* @return The extracted number and unit.
*/
export function getParsedValue( value, unit, units ) {
export function getParsedValue(
value: Value,
unit?: string,
units?: WPUnitControlUnitList
): [ Value, string ] {
const initialValue = unit ? `${ value }${ unit }` : value;

return parseUnit( initialValue, units );
Expand All @@ -171,32 +163,35 @@ export function getParsedValue( value, unit, units ) {
/**
* Checks if units are defined.
*
* @param {any} units Units to check.
* @return {boolean} Whether units are defined.
* @param units Units to check.
* @return Whether units are defined.
*/
export function hasUnits( units ) {
return ! isEmpty( units ) && units !== false;
export function hasUnits( units: WPUnitControlUnitList ): boolean {
return Array.isArray( units ) && !! units.length;
}

/**
* Parses a number and unit from a value.
*
* @param {string} initialValue Value to parse
* @param {Array<WPUnitControlUnit>} units Units to derive from.
* @return {Array<number, string>} The extracted number and unit.
* @param initialValue Value to parse
* @param units Units to derive from.
* @return The extracted number and unit.
*/
export function parseUnit( initialValue, units = ALL_CSS_UNITS ) {
export function parseUnit(
initialValue: Value,
units: WPUnitControlUnitList = ALL_CSS_UNITS
): [ Value, string ] {
const value = String( initialValue ).trim();

let num = parseFloat( value, 10 );
let num: Value = parseFloat( value );
num = isNaN( num ) ? '' : num;

const unitMatch = value.match( /[\d.\-\+]*\s*(.*)/ )[ 1 ];

let unit = unitMatch !== undefined ? unitMatch : '';
unit = unit.toLowerCase();

if ( hasUnits( units ) ) {
if ( hasUnits( units ) && units !== false ) {
const match = units.find( ( item ) => item.value === unit );
unit = match?.value;
} else {
Expand All @@ -210,18 +205,25 @@ export function parseUnit( initialValue, units = ALL_CSS_UNITS ) {
* Parses a number and unit from a value. Validates parsed value, using fallback
* value if invalid.
*
* @param {number|string} next The next value.
* @param {Array<WPUnitControlUnit>} units Units to derive from.
* @param {number|string} fallbackValue The fallback value.
* @param {string} fallbackUnit The fallback value.
* @return {Array<number, string>} The extracted number and unit.
* @param next The next value.
* @param units Units to derive from.
* @param fallbackValue The fallback value.
* @param fallbackUnit The fallback value.
* @return The extracted value and unit.
*/
export function getValidParsedUnit( next, units, fallbackValue, fallbackUnit ) {
export function getValidParsedUnit(
next: Value,
units: WPUnitControlUnitList,
fallbackValue: Value,
fallbackUnit: string
) {
const [ parsedValue, parsedUnit ] = parseUnit( next, units );
let baseValue = parsedValue;
let baseUnit;
let baseUnit: string;

if ( isNaN( parsedValue ) || parsedValue === '' ) {
// The parsed value from `parseUnit` should now be either a
// real number or an empty string. If not, use the fallback value.
if ( ! Number.isFinite( parsedValue ) || parsedValue === '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider this change carefully, since it introduces a runtime change — isNaN doesn't always return the same results as ! Number.isFinite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I meant to leave a specific comment for it. With the switch to TypeScript, using isNaN here generated an error:

image

This seems to be that in TS, isNaN expects to be called with a variable of type number, but our parsedValue is either a number or a string. At this point in getValidParsedUnit the parsedValue variable must be either a number or an empty string (the results of calling parseUnit). So, I think Number.isFinite is a more appropriate check than isNaN anyway, at least from my mental model of what this function should be doing:

  • Check that the number is not NaN
  • Check that the number is a valid value that we can use (e.g. not Infinity or -Infinity) and not an arbitrary string

If it doesn't meet the above criteria then we fall back to the fallbackValue. One difference in using Number.isFinite is that the value won't be coerced to a number before checking to see if it's finite. I think this is okay, because the value has already been coerced in parseUnit so at this point it should be a real (finite) value in order for us to consider it valid.

Please let me know if you think that's not right, though! Happy to change the logic here, I think it's the main place where I had to introduce a runtime change, but 🤞 it isn't an unexpected one in usage.

It could be that we might want to tighten up the Value type a bit further, too, given that in some places it'll only be number or the string literal '' — I was a bit cautious about doing that, as we'd then have two different Value types floating around in the file, which could get confusing? But it also might make it a bit clearer 🤔

Copy link
Contributor

@ciampo ciampo Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please let me know if you think that's not right, though! Happy to change the logic here, I think it's the main place where I had to introduce a runtime change, but 🤞 it isn't an unexpected one in usage.

Thank you for the explanation! With this context (and the extra reassurance given by the passing unit tests), I think we can leave your new implementation untouched.

I would maybe only add a comment with a short version of the explanation you just gave in your previous message.

It could be that we might want to tighten up the Value type a bit further, too, given that in some places it'll only be number or the string literal '' — I was a bit cautious about doing that, as we'd then have two different Value types floating around in the file, which could get confusing? But it also might make it a bit clearer 🤔

I had the same thought and came to the same conclusion

baseValue = fallbackValue;
}

Expand All @@ -242,41 +244,54 @@ export function getValidParsedUnit( next, units, fallbackValue, fallbackUnit ) {
* Takes a unit value and finds the matching accessibility label for the
* unit abbreviation.
*
* @param {string} unit Unit value (example: px)
* @return {string} a11y label for the unit abbreviation
* @param unit Unit value (example: px)
* @return a11y label for the unit abbreviation
*/
export function parseA11yLabelForUnit( unit ) {
export function parseA11yLabelForUnit( unit: string ): string {
const match = ALL_CSS_UNITS.find( ( item ) => item.value === unit );
return match?.a11yLabel ? match?.a11yLabel : match?.value;
}

/**
* Filters available units based on values defined by settings.
* Filters available units based on values defined by the unit setting/property.
*
* @param {Array} settings Collection of preferred units.
* @param {Array} units Collection of available units.
* @param unitSetting Collection of preferred unit value strings.
* @param units Collection of available unit objects.
*
* @return {Array} Filtered units based on settings.
* @return Filtered units based on settings.
*/
export function filterUnitsWithSettings( settings = [], units = [] ) {
return units.filter( ( unit ) => {
return settings.includes( unit.value );
} );
export function filterUnitsWithSettings(
unitSetting: Array< string > = [],
units: WPUnitControlUnitList
): Array< WPUnitControlUnit > {
return Array.isArray( units )
? units.filter( ( unit ) => {
return unitSetting.includes( unit.value );
} )
: [];
}

/**
* Custom hook to retrieve and consolidate units setting from add_theme_support().
* TODO: ideally this hook shouldn't be needed
* https://github.com/WordPress/gutenberg/pull/31822#discussion_r633280823
*
* @param {Object} args An object containing units, settingPath & defaultUnits.
* @param {Array<WPUnitControlUnit>|undefined} args.units Collection of available units.
* @param {Array<string>|undefined} args.availableUnits The setting path. Defaults to 'spacing.units'.
* @param {Object|undefined} args.defaultValues Collection of default values for defined units. Example: { px: '350', em: '15' }.
* @param args An object containing units, settingPath & defaultUnits.
* @param args.units Collection of all potentially available units.
* @param args.availableUnits Collection of unit value strings for filtering available units.
* @param args.defaultValues Collection of default values for defined units. Example: { px: '350', em: '15' }.
*
* @return {Array|boolean} Filtered units based on settings.
* @return Filtered units based on settings.
*/
export const useCustomUnits = ( { units, availableUnits, defaultValues } ) => {
export const useCustomUnits = ( {
units,
availableUnits,
defaultValues,
}: {
units?: WPUnitControlUnitList;
availableUnits?: Array< string >;
defaultValues: Record< string, Value >;
} ): WPUnitControlUnitList => {
units = units || ALL_CSS_UNITS;
const usedUnits = filterUnitsWithSettings(
! availableUnits ? [] : availableUnits,
Expand All @@ -302,17 +317,17 @@ export const useCustomUnits = ( { units, availableUnits, defaultValues } ) => {
* accurately displayed in the UI, even if the intention is to hide
* the availability of that unit.
*
* @param {number|string} currentValue Selected value to parse.
* @param {string} legacyUnit Legacy unit value, if currentValue needs it appended.
* @param {Array<WPUnitControlUnit>} units List of available units.
* @param currentValue Selected value to parse.
* @param legacyUnit Legacy unit value, if currentValue needs it appended.
* @param units List of available units.
*
* @return {Array<WPUnitControlUnit>} A collection of units containing the unit for the current value.
* @return A collection of units containing the unit for the current value.
*/
export function getUnitsWithCurrentUnit(
currentValue,
legacyUnit,
units = ALL_CSS_UNITS
) {
currentValue: Value,
legacyUnit: string | undefined,
units: Array< WPUnitControlUnit > | false = ALL_CSS_UNITS
): WPUnitControlUnitList {
if ( ! Array.isArray( units ) ) {
return units;
}
Expand Down