-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 12 commits
1d515c7
99c864b
d7e95f9
d1954ca
0046f5b
6233279
96f98e3
c1c323b
3959a97
ab43194
9756bdc
043a487
0f1e218
d524a2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
display: flex; | ||
justify-content: space-between; | ||
} | ||
|
||
.blocks-font-size .blocks-range-control__slider + .dashicon { | ||
width: 30px; | ||
height: 30px; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
* External dependencies | ||
*/ | ||
import classnames from 'classnames'; | ||
import { findKey, map } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
|
@@ -14,6 +15,8 @@ import { | |
PanelColor, | ||
RangeControl, | ||
ToggleControl, | ||
Button, | ||
ButtonGroup, | ||
withFallbackStyles, | ||
} from '@wordpress/components'; | ||
|
||
|
@@ -46,6 +49,13 @@ const ContrastCheckerWithFallbackStyles = withFallbackStyles( ( node, ownProps ) | |
}; | ||
} )( ContrastChecker ); | ||
|
||
const FONT_SIZES = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
|
@@ -81,6 +91,12 @@ class ParagraphBlock extends Component { | |
this.nodeRef = node; | ||
} | ||
|
||
setFontSize( fontSize ) { | ||
const { setAttributes } = this.props; | ||
|
||
setAttributes( { fontSize } ); | ||
} | ||
|
||
render() { | ||
const { | ||
attributes, | ||
|
@@ -116,21 +132,47 @@ class ParagraphBlock extends Component { | |
), | ||
isSelected && ( | ||
<InspectorControls key="inspector"> | ||
<PanelBody title={ __( 'Text Settings' ) }> | ||
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I expect we're planning to extract this to a separate component? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. |
||
<div className="blocks-font-size__main"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' ) }> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't we want There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( null ) } | ||
> | ||
{ __( '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 | ||
|
@@ -314,16 +356,29 @@ export const settings = { | |
edit: ParagraphBlock, | ||
|
||
save( { attributes } ) { | ||
const { width, align, content, dropCap, backgroundColor, textColor, fontSize } = attributes; | ||
const { | ||
width, | ||
align, | ||
content, | ||
dropCap, | ||
backgroundColor, | ||
textColor, | ||
fontSize, | ||
} = attributes; | ||
|
||
const thresholdFontSize = findKey( FONT_SIZES, ( size ) => size === fontSize ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing here is that we'd want this to be abstracted away from blocks when we make FontSize a component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this could be a filterLike function
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The attribute contains the font size as a number. I think this raises some problems. If I'm in a theme that sets large font size as 36 and I switch to a theme that sets large fontSize as 37 the blocks with large fontSize created on the old theme will become invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is why I had a separate attribute for the class. The number should only be used when it's a custom value done by the user (and internally to render the inline style). But we should not save anything about the number if it's coming from the predefined size. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not aware of the need to make these thresholds customizable. In that case, I can update the PR to restore the size attribute |
||
|
||
const className = classnames( { | ||
[ `align${ width }` ]: width, | ||
'has-background': backgroundColor, | ||
'has-drop-cap': dropCap, | ||
[ `is-${ thresholdFontSize }-text` ]: thresholdFontSize, | ||
} ); | ||
|
||
const styles = { | ||
backgroundColor: backgroundColor, | ||
color: textColor, | ||
fontSize: fontSize, | ||
fontSize: thresholdFontSize ? null : fontSize, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normally in this situation, I would use thresholdFontSize ? undefined : fontSize,. But in this case, I think using null is safe as false values will be ignored. |
||
textAlign: align, | ||
}; | ||
|
||
|
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe, but I'm cautious about defining styles generically for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
|
||
|
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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_group_role Maybe we need to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.