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

Jump to bottom

Predefined set of font sizes #5269

Merged
merged 14 commits into from Mar 20, 2018
Merged

Predefined set of font sizes #5269

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1d515c7
Add predefined sets of font size values paired with classes.
mtias Feb 24, 2018
99c864b
Load default font-sizes in the front.
mtias Feb 26, 2018
d7e95f9
Add ButtonGroup UI component.
mtias Mar 8, 2018
d1954ca
Use button-group for font-size buttons.
mtias Mar 8, 2018
0046f5b
Use a more sensible max value for the slider.
mtias Mar 8, 2018
6233279
Group reset button with predefined sizes.
mtias Mar 8, 2018
96f98e3
Simplify font size declaration with keyed object.
mtias Mar 8, 2018
c1c323b
Add aria-pressed attr and display afterIcon in range slider.
mtias Mar 8, 2018
3959a97
Remove debugging element.
mtias Mar 8, 2018
ab43194
Simplify rendering redundancy.
mtias Mar 19, 2018
9756bdc
Paragraph Block: Avoid extra block attribute to save the font-size
youknowriad Mar 19, 2018
043a487
Paragraph Block: small code style change in the className generation
youknowriad Mar 19, 2018
0f1e218
Refactored to the two attributes approach customFontSize and fontSize.
jorgefilipecosta Mar 19, 2018
d524a2b
Implemented deprecation logic to convert existing paragraph blocks.
jorgefilipecosta Mar 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions blocks/library/paragraph/editor.scss
View file
Open in desktop
@@ -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;
}
93 changes: 80 additions & 13 deletions blocks/library/paragraph/index.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { findKey, map } 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,6 +49,13 @@ 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 );
Expand Down Expand Up @@ -81,6 +91,23 @@ class ParagraphBlock extends Component {
this.nodeRef = node;
}

setFontSize( fontSize ) {
const { setAttributes } = this.props;
const size = findKey( FONT_SIZES, ( value ) => value === fontSize );

let textClass;
if ( size ) {
textClass = `is-${ size }-text`;
}

setAttributes( { fontSize, textClass } );
}

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

render() {
const {
attributes,
Expand Down Expand Up @@ -116,21 +143,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">
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 whole thing should become a reusable block-component.

<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={ attributes.textClass === `is-${ size }-text` }
aria-pressed={ attributes.textClass === `is-${ size }-text` }
onClick={ () => this.setFontSize( FONT_SIZES[ size ] ) }
>
{ label }
</Button>
) ) }
</ButtonGroup>
<Button
isLarge
onClick={ () => this.setFontSize( null ) }
>
{ __( 'Reset' ) }
</Button>
</div>
<RangeControl
label={ __( 'Custom Size' ) }
value={ this.getFontSize() || '' }
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 @@ -242,6 +295,9 @@ const schema = {
fontSize: {
type: 'number',
},
textClass: {
Copy link
Member

Choose a reason for hiding this comment

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

We have two attributes for font size (fontSize and textClass) both holding information about the size of the text. Plus having a class as attribute seems redundant because it is already in the p tag and it seems it should have been parsed from there.
What about using just one attribute with for the size and when it is a number it is interpreted that a specific font size should be used, when is something like %regular, we assume that a class should be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

and it seems it should have been parsed from there.

I agree.

What about using just one attribute with for the size and when it is a number it is interpreted that a specific font size should be used, when is something like %regular, we assume that a class should be used?

Can you expand a bit on how you see this implemented?

Copy link
Member

Choose a reason for hiding this comment

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

Not certain if it is the test approach or not, but we can have just the fontSize attribute as a string. On save/edit if it starts with '%' like '%regular' we assume it's a "named font size" / class and use it to generate the corresponding class e.g: "is-regular-text". If the fontSize is a number we do the same as we do now.
I used a version of this logic in #5273.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the obscure prefix?

isFinite( '26' )
// true
isFinite( 'regular' )
// false

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we don't need any prefix in this case, in the colors one we needed and I did not made the mental switch.

Copy link
Member

Choose a reason for hiding this comment

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

Even in the color one, can't you identify a color value? What is it, a hex?

Copy link
Member

Choose a reason for hiding this comment

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

In the colors, a color can be 'red', '#ff0', or 'rgb( 255, 0, 0)' and we may want a named color called red but with a value equal to #d82e00 so a special prefix for our named colors seems like the best approach e.g: %red.

Copy link
Member

@aduth aduth Mar 9, 2018 •
edited

Choose a reason for hiding this comment

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

https://github.com/Qix-/color-string/blob/06c3ad42fd60bb8e872a9ef275d3132656bc9c44/index.js#L19-L35

type: 'string',
},
};

export const name = 'core/paragraph';
Expand Down Expand Up @@ -314,16 +370,27 @@ export const settings = {
edit: ParagraphBlock,

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

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

const styles = {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize,
fontSize: textClass ? null : fontSize,
textAlign: align,
};

Expand Down
38 changes: 28 additions & 10 deletions blocks/library/paragraph/style.scss
View file
Open in desktop
@@ -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

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
View file
Open in desktop
@@ -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
View file
Open in desktop
@@ -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
View file
Open in desktop
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