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

Predefined set of font sizes #5269

Merged
merged 14 commits into from
Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 10 additions & 0 deletions blocks/library/paragraph/editor.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph {
background: white;
}

.blocks-font-size__main {
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 will be extracted into a block component.

display: flex;
justify-content: space-between;
}

.blocks-font-size .blocks-range-control__slider + .dashicon {
width: 30px;
height: 30px;
}
141 changes: 128 additions & 13 deletions blocks/library/paragraph/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { findKey, isFinite, map, omit } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -14,6 +15,8 @@ import {
PanelColor,
RangeControl,
ToggleControl,
Button,
ButtonGroup,
withFallbackStyles,
} from '@wordpress/components';

Expand Down Expand Up @@ -46,13 +49,22 @@ const ContrastCheckerWithFallbackStyles = withFallbackStyles( ( node, ownProps )
};
} )( ContrastChecker );

const FONT_SIZES = {
Copy link
Member

Choose a reason for hiding this comment

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

Should we try to read this values from the theme, e.g: provide and extensibility mechanism for themes to change them, or use invisible elements plus getComputedStyles to check if themes css changes our styles?

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely allow a theme to set this, but should be done separately

small: 14,
regular: 16,
large: 36,
larger: 48,
};

class ParagraphBlock extends Component {
constructor() {
super( ...arguments );
this.nodeRef = null;
this.bindRef = this.bindRef.bind( this );
this.onReplace = this.onReplace.bind( this );
this.toggleDropCap = this.toggleDropCap.bind( this );
this.getFontSize = this.getFontSize.bind( this );
this.setFontSize = this.setFontSize.bind( this );
}

onReplace( blocks ) {
Expand Down Expand Up @@ -81,6 +93,33 @@ class ParagraphBlock extends Component {
this.nodeRef = node;
}

getFontSize() {
const { customFontSize, fontSize } = this.props.attributes;
if ( fontSize ) {
return FONT_SIZES[ fontSize ];
}

if ( customFontSize ) {
return customFontSize;
}
}

setFontSize( fontSizeValue ) {
const { setAttributes } = this.props;
const thresholdFontSize = findKey( FONT_SIZES, ( size ) => size === fontSizeValue );
if ( thresholdFontSize ) {
setAttributes( {
fontSize: thresholdFontSize,
customFontSize: undefined,
} );
return;
}
setAttributes( {
fontSize: undefined,
customFontSize: fontSizeValue,
} );
}

render() {
const {
attributes,
Expand All @@ -97,12 +136,13 @@ class ParagraphBlock extends Component {
content,
dropCap,
placeholder,
fontSize,
backgroundColor,
textColor,
width,
} = attributes;

const fontSize = this.getFontSize();

return [
isSelected && (
<BlockControls key="controls">
Expand All @@ -116,21 +156,47 @@ class ParagraphBlock extends Component {
),
isSelected && (
<InspectorControls key="inspector">
<PanelBody title={ __( 'Text Settings' ) }>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">
Copy link
Member

Choose a reason for hiding this comment

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

I expect we're planning to extract this to a separate component?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed.

<div className="blocks-font-size__main">
<ButtonGroup aria-label={ __( 'Font Size' ) }>
Copy link
Member

Choose a reason for hiding this comment

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

Won't we want aria-label on each of the button options instead for "Small", "Medium", etc. ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed this and opted for not having one for now.

{ map( {
S: 'small',
M: 'regular',
L: 'large',
XL: 'larger',
}, ( size, label ) => (
<Button
key={ label }
isLarge
isPrimary={ fontSize === FONT_SIZES[ size ] }
aria-pressed={ fontSize === FONT_SIZES[ size ] }
onClick={ () => this.setFontSize( FONT_SIZES[ size ] ) }
>
{ label }
</Button>
) ) }
</ButtonGroup>
<Button
isLarge
onClick={ () => this.setFontSize( undefined ) }
>
{ __( 'Reset' ) }
</Button>
</div>
<RangeControl
label={ __( 'Custom Size' ) }
value={ fontSize || '' }
onChange={ ( value ) => this.setFontSize( value ) }
min={ 12 }
max={ 100 }
beforeIcon="editor-textcolor"
afterIcon="editor-textcolor"
/>
<ToggleControl
label={ __( 'Drop Cap' ) }
checked={ !! dropCap }
onChange={ this.toggleDropCap }
/>
<RangeControl
label={ __( 'Font Size' ) }
value={ fontSize || '' }
onChange={ ( value ) => setAttributes( { fontSize: value } ) }
min={ 10 }
max={ 200 }
beforeIcon="editor-textcolor"
allowReset
/>
</PanelBody>
<PanelColor title={ __( 'Background Color' ) } colorValue={ backgroundColor } initialOpen={ false }>
<ColorPalette
Expand Down Expand Up @@ -240,6 +306,9 @@ const schema = {
type: 'string',
},
fontSize: {
type: 'string',
},
customFontSize: {
type: 'number',
},
};
Expand Down Expand Up @@ -275,6 +344,40 @@ export const settings = {
},

deprecated: [
{
supports,
attributes: omit( {
...schema,
fontSize: {
type: 'number',
},
}, 'customFontSize' ),
save( { attributes } ) {
const { width, align, content, dropCap, backgroundColor, textColor, fontSize } = attributes;
const className = classnames( {
[ `align${ width }` ]: width,
'has-background': backgroundColor,
'has-drop-cap': dropCap,
} );
const styles = {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize,
textAlign: align,
};

return <p style={ styles } className={ className ? className : undefined }>{ content }</p>;
},
migrate( attributes ) {
if ( isFinite( attributes.fontSize ) ) {
return omit( {
...attributes,
customFontSize: attributes.fontSize,
}, 'fontSize' );
}
return attributes;
},
},
{
supports,
attributes: {
Expand Down Expand Up @@ -314,16 +417,28 @@ export const settings = {
edit: ParagraphBlock,

save( { attributes } ) {
const { width, align, content, dropCap, backgroundColor, textColor, fontSize } = attributes;
const {
width,
align,
content,
dropCap,
backgroundColor,
textColor,
fontSize,
customFontSize,
} = attributes;

const className = classnames( {
[ `align${ width }` ]: width,
'has-background': backgroundColor,
'has-drop-cap': dropCap,
[ `is-${ fontSize }-text` ]: fontSize && FONT_SIZES[ fontSize ],
} );

const styles = {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize,
fontSize: ! fontSize && customFontSize ? customFontSize : undefined,
textAlign: align,
};

Expand Down
38 changes: 28 additions & 10 deletions blocks/library/paragraph/style.scss
Original file line number Diff line number Diff line change
@@ -1,13 +1,31 @@
p.has-drop-cap {
&:first-letter {
float: left;
font-size: 4.1em;
line-height: 0.7;
font-family: serif;
font-weight: bold;
margin: .07em .23em 0 0;
text-transform: uppercase;
font-style: normal;
p {
&.is-small-text {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Mar 8, 2018

Choose a reason for hiding this comment

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

Should this classes (is-small-text, is-regular-text) etc... be outside of paragraph so other blocks can make use of our font size mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe, but I'm cautious about defining styles generically for *.is-small-text. Also, in some other blocks reusing this component, they might want to have more fine-grained targets.

Copy link
Member

Choose a reason for hiding this comment

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

So this is a problem because it is applying outside the paragraph block. There's no scoping on the p selector to make this specific to the paragraph block.

font-size: 14px;
}

&.is-regular-text {
font-size: 16px;
}

&.is-large-text {
font-size: 36px;
}

&.is-larger-text {
font-size: 48px;
}

&.has-drop-cap {
&:first-letter {
Copy link
Member

Choose a reason for hiding this comment

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

Nesting necessary? vs.

&.has-drop-cap:first-letter {

}

float: left;
font-size: 4.1em;
line-height: 0.7;
font-family: serif;
font-weight: bold;
margin: .07em .23em 0 0;
text-transform: uppercase;
font-style: normal;
}
}
}

Expand Down
19 changes: 19 additions & 0 deletions components/button-group/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* Internal dependencies
*/
import './style.scss';

function ButtonGroup( { className, ...props } ) {
const classes = classnames( 'components-button-group', className );

return (
<div { ...props } className={ classes } role="group" />
Copy link
Member

Choose a reason for hiding this comment

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

A group should be used to form a logical collection of items with related functionality, such as children in a tree widget forming a collection of siblings in a hierarchy, or a collection of items having the same container in a directory. However, when a group is used in the context of list, authors must limit its children to listitem elements. Group elements may be nested.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_group_role

Maybe we need to Children.map to apply a wrapper to each child?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed in slack channel and markup seemed good as is.

);
}

export default ButtonGroup;
19 changes: 19 additions & 0 deletions components/button-group/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
.components-button-group {
display: inline-block;
margin-bottom: 20px;

.components-button {
border-radius: 0;
& + .components-button {
margin-left: -1px;
}

&:first-child {
border-radius: 3px 0 0 3px;
}

&:last-child {
border-radius: 0 3px 3px 0;
}
}
}
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export { default as APIProvider } from './higher-order/with-api-data/provider';
export { default as Autocomplete } from './autocomplete';
export { default as BaseControl } from './base-control';
export { default as Button } from './button';
export { default as ButtonGroup } from './button-group';
export { default as CheckboxControl } from './checkbox-control';
export { default as ClipboardButton } from './clipboard-button';
export { default as CodeEditor } from './code-editor';
Expand Down