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

Font Size Picker: Allow non-integers as simple CSS values and in hints #36636

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Font Size Picker: Allow non-integers as simple CSS values and in hints #36636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
347ea3a
Remove hints and allow non-integers
aaronrobertshaw Nov 19, 2021
3f39aa9
Reinstate hints
aaronrobertshaw Nov 26, 2021
0d6544f
Remove Dynamic label from header hints
aaronrobertshaw Nov 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
38 changes: 38 additions & 0 deletions packages/components/src/font-size-picker/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,41 @@ export const differentControlBySize = () => {
<FontSizePickerWithState fontSizes={ fontSizes } initialValue={ 8 } />
);
};

export const withComplexCSSValues = () => {
const fontSizes = object( 'Font Sizes', [
{
name: 'Small',
slug: 'small',
size: '0.75rem',
},
{
name: 'Normal',
slug: 'normal',
size: '1rem',
},
{
name: 'Large',
slug: 'large',
size: '2.5rem',
},
{
name: 'Extra Large',
slug: 'extra-large',
size: '3.5rem',
},
{
name: 'Huge',
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
slug: 'huge',
size: 'clamp(2.5rem, 4vw, 3rem)',
},
] );
return (
<div style={ { maxWidth: '248px' } }>
<FontSizePickerWithState
fontSizes={ fontSizes }
initialValue={ '1rem' }
/>
</div>
);
};
74 changes: 74 additions & 0 deletions packages/components/src/font-size-picker/test/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* Internal dependencies
*/
import { isSimpleCssValue, splitValueAndUnitFromSize } from '../utils';

const simpleCSSCases = [
// Test integers and non-integers.
[ 1, true ],
[ 1.25, true ],
[ '123', true ],
[ '1.5', true ],
[ '0.75', true ],
// CSS unit tests.
[ '20px', true ],
[ '0.8em', true ],
[ '2rem', true ],
[ '1.4vw', true ],
[ '0.4vh', true ],
// Invalid negative values,
[ '-5px', false ],
// Complex CSS values that should fail.
[ 'abs(-10px)', false ],
[ 'calc(10px + 1)', false ],
[ 'clamp(2.5rem, 4vw, 3rem)', false ],
[ 'max(4.5em, 3vh)', false ],
[ 'min(10px, 1rem)', false ],
[ 'minmax(30px, auto)', false ],
[ 'var(--wp--font-size)', false ],
];

describe( 'isSimpleCssValue', () => {
test.each( simpleCSSCases )(
'given %p as argument, returns %p',
( cssValue, result ) => {
expect( isSimpleCssValue( cssValue ) ).toBe( result );
}
);
} );

const splitValuesCases = [
// Test integers and non-integers.
[ 1, '1', undefined ],
[ 1.25, '1.25', undefined ],
[ '123', '123', undefined ],
[ '1.5', '1.5', undefined ],
[ '0.75', '0.75', undefined ],
// Valid simple CSS values.
[ '20px', '20', 'px' ],
[ '0.8em', '0.8', 'em' ],
[ '2rem', '2', 'rem' ],
[ '1.4vw', '1.4', 'vw' ],
[ '0.4vh', '0.4', 'vh' ],
// Invalid negative values,
[ '-5px', undefined, undefined ],
// Complex CSS values that shouldn't parse.
[ 'abs(-15px)', undefined, undefined ],
[ 'calc(10px + 1)', undefined, undefined ],
[ 'clamp(2.5rem, 4vw, 3rem)', undefined, undefined ],
[ 'max(4.5em, 3vh)', undefined, undefined ],
[ 'min(10px, 1rem)', undefined, undefined ],
[ 'minmax(30px, auto)', undefined, undefined ],
[ 'var(--wp--font-size)', undefined, undefined ],
];

describe( 'splitValueAndUnitFromSize', () => {
test.each( splitValuesCases )(
'given %p as argument, returns value = %p and unit = %p',
( cssValue, expectedValue, expectedUnit ) => {
const [ value, unit ] = splitValueAndUnitFromSize( cssValue );
expect( value ).toBe( expectedValue );
expect( unit ).toBe( expectedUnit );
}
);
} );
17 changes: 9 additions & 8 deletions packages/components/src/font-size-picker/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ const CUSTOM_FONT_SIZE_OPTION = {
* @return {[number, string]} An array with the numeric value and the unit if exists.
*/
export function splitValueAndUnitFromSize( size ) {
/**
* The first matched result is ignored as it's the left
* hand side of the capturing group in the regex.
*/
const [ , numericValue, unit ] = size.split( /(\d+)/ );
return [ numericValue, unit ];
const [ numericValue, unit ] = `${ size }`.match( /[\d\.]+|\D+/g );
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the changes made in #36420, I believe that we should also apply the change on the JSDoc for the size param (i.e. changing its type to be only string)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes here were slightly different to #36420. It honours what was documented in terms of the param accepting string|number. The tests also cover both strings and numbers.

The FontSizePicker docs themselves also include numerical size values. Should we not maintain compatibility with that?

In light of that, do you still believe I should restrict this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You actually made a few good points — let's leave it as-is (and thank you for looking into it)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering why you changed from split to match. I was expecting a change in the regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, this wasn't a conscious choice. It came from when I copied the original change.

On a closer look though, match appears a little cleaner in terms of its results. I don't feel strongly about it, so if there is a good reason to change it back to split I can.


if ( ! isNaN( parseFloat( numericValue ) ) && isFinite( numericValue ) ) {
return [ numericValue, unit ];
}

return [];
}

/**
Expand All @@ -38,7 +39,7 @@ export function splitValueAndUnitFromSize( size ) {
* @return {boolean} Whether the value is a simple css value.
*/
export function isSimpleCssValue( value ) {
const sizeRegex = /^(?!0)\d+(px|em|rem|vw|vh|%)?$/i;
const sizeRegex = /^[\d\.]+(px|em|rem|vw|vh|%)?$/i;
return sizeRegex.test( value );
}

Expand Down Expand Up @@ -75,7 +76,7 @@ function getSelectOptions( optionsArray, disableCustomFontSizes ) {
name,
size,
__experimentalHint:
size && isSimpleCssValue( size ) && parseInt( size ),
size && isSimpleCssValue( size ) && parseFloat( size ),
} ) );
}

Expand Down