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

Set font size slider to 1 as default, em units #3090

Closed
wants to merge 3 commits into from
Closed

Set font size slider to 1 as default, em units #3090

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12f49ab
Set font size slider to 1 as default, em units
notnownikki Oct 20, 2017
fb6c7cc
Update MAX_FONT_SIZE
notnownikki Oct 20, 2017
228620c
Added span to set relative font size, and new API for matching on mul…
notnownikki Oct 27, 2017
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
3 changes: 2 additions & 1 deletion blocks/inspector-controls/range-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { __ } from '@wordpress/i18n';
import BaseControl from './../base-control';
import './style.scss';

function RangeControl( { label, value, instanceId, onChange, beforeIcon, afterIcon, help, allowReset, ...props } ) {
function RangeControl( { label, value, instanceId, onChange, beforeIcon, afterIcon, help, allowReset, units, ...props } ) {
const id = 'inspector-range-control-' + instanceId;
const onChangeValue = ( event ) => onChange( Number( event.target.value ) );

Expand All @@ -33,6 +33,7 @@ function RangeControl( { label, value, instanceId, onChange, beforeIcon, afterIc
value={ value }
{ ...props }
/>
{ units }
{ allowReset &&
<Button onClick={ () => onChange() } disabled={ value === undefined }>
{ __( 'Reset' ) }
Expand Down
21 changes: 14 additions & 7 deletions blocks/library/paragraph/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import ColorPalette from '../../color-palette';
import BlockDescription from '../../block-description';

const { children } = source;
const MAX_FONT_SIZE = 9;

registerBlockType( 'core/paragraph', {
title: __( 'Paragraph' ),
Expand Down Expand Up @@ -95,10 +96,14 @@ registerBlockType( 'core/paragraph', {
},

edit( { attributes, setAttributes, insertBlocksAfter, focus, setFocus, mergeBlocks, onReplace } ) {
const { align, content, dropCap, placeholder, fontSize, backgroundColor, textColor, width } = attributes;
const { align, content, dropCap, placeholder, backgroundColor, textColor, width } = attributes;
const toggleDropCap = () => setAttributes( { dropCap: ! dropCap } );
const className = dropCap ? 'has-drop-cap' : null;

let { fontSize } = attributes;
// if the font size was previously set in pixels, divide by 16 to get em
if ( fontSize > MAX_FONT_SIZE ) {
fontSize = Math.max( MAX_FONT_SIZE, fontSize / 16 );
}
return [
focus && (
<BlockControls key="controls">
Expand All @@ -123,11 +128,13 @@ registerBlockType( 'core/paragraph', {
/>
<RangeControl
label={ __( 'Font Size' ) }
value={ fontSize || '' }
value={ fontSize || 1 }
onChange={ ( value ) => setAttributes( { fontSize: value } ) }
min={ 10 }
max={ 200 }
min={ 0.5 }
max={ MAX_FONT_SIZE }
step={ 0.1 }
beforeIcon="editor-textcolor"
units="em"
allowReset
/>
</PanelBody>
Expand Down Expand Up @@ -162,7 +169,7 @@ registerBlockType( 'core/paragraph', {
style={ {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize ? fontSize + 'px' : undefined,
fontSize: fontSize ? fontSize + 'em' : undefined,
textAlign: align,
} }
value={ content }
Expand Down Expand Up @@ -198,7 +205,7 @@ registerBlockType( 'core/paragraph', {
const styles = {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize,
fontSize: fontSize ? fontSize + 'em' : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the theme sets a font size on the paragraph in a stylesheet? Then it would base the font size on the parent node, right? https://developer.mozilla.org/en-US/docs/Web/CSS/font-size#Ems

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's why I thought that em was the right choice. The theme should be setting the base font size, and the font size slider is there to make it bigger or smaller. Or am I misunderstanding the issue?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, em works in that way, but ideally it would be relative to the normal font size of that element, rather than the font size of the parent element. See #2610 (comment). Many themes target the paragraph directly, e.g. .content p { font-size: 16px }. In this case, I think it would break?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure... from what I can see, it would be fine. Here's how we currently set the font size on paragraphs in the editor:

.editor-visual-editor p {
    font-family: "Noto Serif", serif;
    font-size: 16px;
    line-height: 1.8;
}

Applying a font-size in the paragraph's style attribute seems to accept 16px as the same as 1em, and it works as I expected. (I compared an unstyled paragraph to one with font-size set to 1em, they were identical) Perhaps I'm missing some detail though?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was missing a detail. It was coincidence it worked. Thanks for putting me on the right track here.

textAlign: align,
};

Expand Down