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

Updates to the Font Size Picker #9802

Merged
merged 11 commits into from
Oct 4, 2018
Merged

Updates to the Font Size Picker #9802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
713526e
Rework font-size selection to improve clarity of options and the UI.
mtias Sep 10, 2018
16a84de
Add label, relax margins, tighten help text for switch, fix color
Sep 11, 2018
3e7c044
Tweak the reset button, make it disabled until a number is input.
Sep 11, 2018
3ab378d
Fix reset clearing input value.
mtias Sep 11, 2018
af3dfc3
Use different sizes for buttons and render.
mtias Sep 11, 2018
132680f
Convert font picker to a dropdown select showing real visual sizes.
mtias Sep 28, 2018
7848506
Remove unusued CSS code.
mtias Sep 28, 2018
5f2da9a
FontSizePicker: a11y: Wrap with NavigableMenu
mcsf Oct 3, 2018
18ba6db
e2e: Update to accommodate new font size picker
mcsf Oct 3, 2018
5cf9607
Amend editor-modes test
mcsf Oct 3, 2018
62cb488
Accessibility improvements to the FontSizePicker component
youknowriad Oct 4, 2018
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
4 changes: 2 additions & 2 deletions edit-post/components/sidebar/block-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
margin: 0 -16px;

.components-base-control {
margin: 0 0 1em 0;
margin: 0 0 1.5em 0;
&:last-child {
margin-bottom: 0;
margin-bottom: 0.5em;
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/block-library/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,17 @@
}

.has-small-font-size {
font-size: 14px;
font-size: 13px;
}

.has-regular-font-size {
font-size: 16px;
font-size: 20px;
}

.has-large-font-size {
font-size: 36px;
}

.has-larger-font-size {
font-size: 48px;
font-size: 42px;
}
3 changes: 2 additions & 1 deletion packages/components/src/base-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ function BaseControl( { id, label, help, className, children } ) {
return (
<div className={ classnames( 'components-base-control', className ) }>
<div className="components-base-control__field">
{ label && <label className="components-base-control__label" htmlFor={ id }>{ label }</label> }
{ label && id && <label className="components-base-control__label" htmlFor={ id }>{ label }</label> }
{ label && ! id && <span className="components-base-control__label">{ label }</span> }
Copy link
Member

Choose a reason for hiding this comment

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

The use-case does not feel legitimate. I don't think we should support BaseControl with a label and not an id. The label should always direct to an input, even in the case of the font size picker.

This leaves open the possibility for error like described at https://github.com/WordPress/gutenberg/pull/10925/files#r257248594 .

{ children }
</div>
{ !! help && <p id={ id + '__help' } className="components-base-control__help">{ help }</p> }
Expand Down
27 changes: 14 additions & 13 deletions packages/components/src/base-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
.components-base-control {
font-family: $default-font;
font-size: $default-font-size;
}

.components-base-control__field {
margin-bottom: $grid-size;
.components-base-control__field {
margin-bottom: $grid-size;

.components-panel__row & {
margin-bottom: inherit;
.components-panel__row & {
margin-bottom: inherit;
}
}
}

.components-base-control__label {
display: block;
margin-bottom: $grid-size-small;
}
.components-base-control__label {
display: block;
margin-bottom: $grid-size-small;
}

.components-base-control__help {
font-style: italic;
margin-bottom: 0;
.components-base-control__help {
margin-top: -$grid-size;
font-style: italic;
margin-bottom: 0;
}
}
111 changes: 85 additions & 26 deletions packages/components/src/font-size-picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,51 +6,110 @@ import { map } from 'lodash';
/**
* WordPress dependencies
*/
import { Fragment } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { withInstanceId } from '@wordpress/compose';

/**
* Internal dependencies
*/
import Dashicon from '../dashicon';
import BaseControl from '../base-control';
import Button from '../button';
import ButtonGroup from '../button-group';
import Dropdown from '../dropdown';
import RangeControl from '../range-control';
import { NavigableMenu } from '../navigable-container';

function FontSizePicker( {
fallbackFontSize,
fontSizes = [],
onChange,
value,
withSlider,
} ) {
const onChangeValue = ( event ) => {
const newValue = event.target.value;
if ( newValue === '' ) {
onChange( undefined );
return;
}
onChange( Number( newValue ) );
};

const currentFont = fontSizes.find( ( font ) => font.size === value );

export default function FontSizePicker( { fontSizes = [], fallbackFontSize, value, onChange } ) {
return (
<Fragment>
<BaseControl label={ __( 'Font Size' ) }>
<div className="components-font-size-picker__buttons">
<ButtonGroup aria-label={ __( 'Font Size' ) }>
{ map( fontSizes, ( { name, size, shortName } ) => (
<Dropdown
className="components-font-size-picker__dropdown"
contentClassName="components-font-size-picker__dropdown-content"
position="bottom"
renderToggle={ ( { isOpen, onToggle } ) => (
<Button
key={ size }
className="components-font-size-picker__selector"
isLarge
isPrimary={ value === size }
aria-pressed={ value === size }
onClick={ () => onChange( size ) }
onClick={ onToggle }
aria-expanded={ isOpen }
aria-label={ __( 'Custom font size' ) }
>
{ shortName || name }
{ ( currentFont && currentFont.name ) || ( ! value && 'Normal' ) || 'Custom' }
</Button>
) ) }
</ButtonGroup>
) }
renderContent={ () => (
<NavigableMenu>
{ map( fontSizes, ( { name, size, slug } ) => (
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

As the NavigableMenu uses a role="menu", all the menu items within it should have a role="menuitem".

Copy link
Contributor

@mcsf mcsf Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It'd be great to have a lint rule that checks for role="menu" in children of any NavigableMenu.

key={ slug }
onClick={ () => onChange( slug === 'normal' ? undefined : size ) }
className={ 'is-font-' + slug }
role="menuitem"
>
{ ( value === size || ( ! value && slug === 'normal' ) ) && <Dashicon icon="saved" /> }
<span className="components-font-size-picker__dropdown-text-size" style={ { fontSize: size } }>
{ name }
</span>
</Button>
) ) }
</NavigableMenu>
) }
/>
{ ! withSlider &&
<input
className="components-range-control__number"
type="number"
onChange={ onChangeValue }
aria-label={ __( 'Custom font size' ) }
value={ value || '' }
/>
}
<Button
isLarge
className="components-color-palette__clear"
type="button"
disabled={ value === undefined }
onClick={ () => onChange( undefined ) }
isButton
isSmall
isDefault
aria-label={ __( 'Reset font size' ) }
>
{ __( 'Reset' ) }
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
</Button>
</div>
<RangeControl
className="components-font-size-picker__custom-input"
label={ __( 'Custom Size' ) }
value={ value || '' }
initialPosition={ fallbackFontSize }
onChange={ onChange }
min={ 12 }
max={ 100 }
beforeIcon="editor-textcolor"
afterIcon="editor-textcolor"
/>
</Fragment>
{ withSlider &&
<RangeControl
className="components-font-size-picker__custom-input"
label={ __( 'Custom Size' ) }
value={ value || '' }
initialPosition={ fallbackFontSize }
onChange={ onChange }
min={ 12 }
max={ 100 }
beforeIcon="editor-textcolor"
afterIcon="editor-textcolor"
/>
}
</BaseControl>
);
}

export default withInstanceId( FontSizePicker );
49 changes: 48 additions & 1 deletion packages/components/src/font-size-picker/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
.components-font-size-picker__buttons {
display: flex;
justify-content: space-between;
margin-bottom: 1em;
align-items: center;

// Apply the same height as the isSmall Reset button.
.components-range-control__number {
height: 24px;
line-height: 22px;

// Show the reset button as disabled until a value is entered.
&[value=""] + .components-button {
cursor: default;
opacity: 0.3;
pointer-events: none;
}
}
}

.components-font-size-picker__custom-input {
Expand All @@ -10,3 +23,37 @@
height: 30px;
}
}

.components-font-size-picker__dropdown-content .components-button {
display: block;
position: relative;
padding: 10px 20px 10px 40px;
width: 100%;
text-align: left;

.dashicon {
position: absolute;
top: calc(50% - 10px);
left: 10px;
}
}

.components-font-size-picker__buttons .components-font-size-picker__selector {
border: 1px solid;
background: none;
position: relative;
width: 110px;

@include input-style__neutral();

&:focus {
@include input-style__focus();
}

&::after {
@include dropdown-arrow();
right: 8px;
top: 12px;
position: absolute;
}
}
19 changes: 10 additions & 9 deletions packages/editor/src/store/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,27 +81,28 @@ export const EDITOR_SETTINGS_DEFAULTS = {
fontSizes: [
{
name: __( 'Small' ),
shortName: __( 'S' ),
size: 14,
size: 13,
slug: 'small',
},
{
name: __( 'Regular' ),
shortName: __( 'M' ),
name: __( 'Normal' ),
size: 16,
slug: 'regular',
slug: 'normal',
},
{
name: __( 'Medium' ),
size: 20,
slug: 'medium',
},
{
name: __( 'Large' ),
shortName: __( 'L' ),
size: 36,
slug: 'large',
},
{
name: __( 'Larger' ),
shortName: __( 'XL' ),
name: __( 'Huge' ),
size: 48,
slug: 'larger',
slug: 'huge',
},
],

Expand Down
3 changes: 2 additions & 1 deletion test/e2e/specs/editor-modes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ describe( 'Editing modes (visual/HTML)', () => {
expect( htmlBlockContent ).toEqual( '<p>Hello world!</p>' );

// Change the font size using the sidebar.
const changeSizeButton = await page.waitForXPath( '//button[text()="L"]' );
await page.click( '.components-font-size-picker__selector' );
const changeSizeButton = await page.waitForSelector( '.components-button.is-font-large' );
await changeSizeButton.click();

// Make sure the HTML content updated.
Expand Down
17 changes: 11 additions & 6 deletions test/e2e/specs/font-size-picker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ describe( 'Font Size Picker', () => {
await clickBlockAppender();
await page.keyboard.type( 'Paragraph to be made "large"' );

const largeButton = ( await page.$x( '//*[contains(concat(" ", @class, " "), " components-font-size-picker__buttons ")]//*[text()=\'L\']' ) )[ 0 ];
await largeButton.click();
await page.click( '.components-font-size-picker__selector' );
const changeSizeButton = await page.waitForSelector( '.components-button.is-font-large' );
await changeSizeButton.click();

// Ensure content matches snapshot.
const content = await getEditedPostContent();
Expand All @@ -31,7 +32,7 @@ describe( 'Font Size Picker', () => {
await page.keyboard.type( 'Paragraph to be made "small"' );

await page.click( '.blocks-font-size .components-range-control__number' );
await page.keyboard.type( '14' );
await page.keyboard.type( '13' );

// Ensure content matches snapshot.
const content = await getEditedPostContent();
Expand All @@ -57,7 +58,10 @@ describe( 'Font Size Picker', () => {
await page.keyboard.type( 'Paragraph with font size reset using button' );

await page.click( '.blocks-font-size .components-range-control__number' );
await page.keyboard.type( '14' );
await page.keyboard.type( '13' );

// Blur the range control
await page.click( '.components-base-control__label' );

const resetButton = ( await page.$x( '//*[contains(concat(" ", @class, " "), " components-font-size-picker__buttons ")]//*[text()=\'Reset\']' ) )[ 0 ];
await resetButton.click();
Expand All @@ -72,8 +76,9 @@ describe( 'Font Size Picker', () => {
await clickBlockAppender();
await page.keyboard.type( 'Paragraph with font size reset using input field' );

const largeButton = ( await page.$x( '//*[contains(concat(" ", @class, " "), " components-font-size-picker__buttons ")]//*[text()=\'L\']' ) )[ 0 ];
await largeButton.click();
await page.click( '.components-font-size-picker__selector' );
const changeSizeButton = await page.waitForSelector( '.components-button.is-font-large' );
await changeSizeButton.click();

await page.click( '.blocks-font-size .components-range-control__number' );
await page.keyboard.press( 'Backspace' );
Expand Down