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

Add basic heading block #423

Merged
merged 5 commits into from
Apr 17, 2017
Merged

Add basic heading block #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2849506
Add basic heading block
ellatrix Apr 14, 2017
8502267
Add buttons to change heading level
ellatrix Apr 14, 2017
6cc0116
Use heading icon
ellatrix Apr 17, 2017
1b0f6c4
Use instead of for IconButton text
ellatrix Apr 17, 2017
f90d96e
Remove editor instance when tags change
ellatrix Apr 17, 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
10 changes: 9 additions & 1 deletion blocks/components/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,22 @@ export default class Editable extends wp.element.Component {
this.editor.selection.moveToBookmark( bookmark );
}

componentWillUpdate( nextProps ) {
if ( this.editor && this.props.tagName !== nextProps.tagName ) {
this.editor.destroy();
}
}

componentWillUnmount() {
if ( this.editor ) {
this.editor.destroy();
}
}

componentDidUpdate( prevProps ) {
if ( this.props.value !== prevProps.value ) {
if ( this.props.tagName !== prevProps.tagName ) {
this.initialize();
} else if ( this.props.value !== prevProps.value ) {
this.updateContent();
}
}
Expand Down
56 changes: 56 additions & 0 deletions blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* Internal dependencies
*/
import { registerBlock, query } from 'api';
import Editable from 'components/editable';

const { html, prop } = query;

registerBlock( 'core/heading', {
title: wp.i18n.__( 'Heading' ),

icon: 'heading',

category: 'common',

attributes: {
content: html( 'h1,h2,h3,h4,h5,h6' ),
tag: prop( 'h1,h2,h3,h4,h5,h6', 'nodeName' ),
align: prop( 'h1,h2,h3,h4,h5,h6', 'style.textAlign' )
},

controls: [
...'123456'.split( '' ).map( ( level ) => ( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need [ ...array ]. We could just use array

Copy link
Member Author

Choose a reason for hiding this comment

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

Future controls?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

icon: 'heading',
text: level,
title: wp.i18n.sprintf( wp.i18n.__( 'Heading %s' ), level ),
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 use %d placeholder instead of %s?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good too. Does it matter much? Sorry for my ignorance.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good too. Does it matter much? Sorry for my ignorance.

I'll not claim to be a localization expert, but perhaps as a hint to the translator that it's expected to be translated with a number vs. any arbitrary string could affect the translated text. Also prevents said arbitrary strings if we're very intentional that it's not to be expected (as a result of some potentially buggy code).

isActive: ( { tag } ) => 'H' + level === tag,
onClick( attributes, setAttributes ) {
setAttributes( { tag: 'H' + level } );
}
} ) )
],

edit( { attributes, setAttributes } ) {
const { content, tag, align } = attributes;

return (
<Editable
tagName={ tag }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
style={ align ? { textAlign: align } : null }
/>
);
},

save( { attributes } ) {
const { align, tag: Tag, content } = attributes;

return (
<Tag
style={ align ? { textAlign: align } : null }
dangerouslySetInnerHTML={ { __html: content } } />
);
}
} );
1 change: 1 addition & 0 deletions blocks/library/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import './freeform';
import './heading';
import './image';
import './text';
import './list';
3 changes: 2 additions & 1 deletion editor/components/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ import './style.scss';
import Button from '../button';
import Dashicon from '../dashicon';

function IconButton( { icon, label, className, ...additionalProps } ) {
function IconButton( { icon, children, label, className, ...additionalProps } ) {
const classes = classnames( 'editor-icon-button', className );

return (
<Button { ...additionalProps } aria-label={ label } className={ classes }>
<Dashicon icon={ icon } />
{ children ? <span>{ children }</span> : null }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the span wrapper on children?

</Button>
);
}
Expand Down
6 changes: 5 additions & 1 deletion editor/components/icon-button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.editor-icon-button {
display: flex;
align-items: center;
width: 36px;
min-width: 36px;
height: 36px;
border: none;
background: none;
Expand All @@ -14,4 +14,8 @@
color: $blue-medium;
}
}

span {
font-size: 12px;
Copy link
Member

Choose a reason for hiding this comment

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

This styling seems very targeted at the heading use-case. Is IconButton with small adjacent text a common enough pattern it should be engrained into the IconButton component itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great question to be asking. The icon-button should be just that, an icon button, so any text added should not so much be a label as it should be something to go with the icon itself. In this case, implying a heading size. But it could also be used for quote styles, or possibly even gallery styles (i.e. quote 2 or gallery 3).

Tying together with your comment #436 (review), I will open a PR to add a label that makes this clear.

}
}
4 changes: 3 additions & 1 deletion editor/components/toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ function Toolbar( { controls } ) {
} }
className={ classNames( 'editor-toolbar__control', {
'is-active': control.isActive && control.isActive()
} ) } />
} ) }>
{ control.text }
</IconButton>
) ) }
</ul>
);
Expand Down
8 changes: 8 additions & 0 deletions languages/gutenberg.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ msgstr ""
msgid "Freeform"
msgstr ""

#: blocks/library/heading/index.js:10
msgid "Heading"
msgstr ""

#: blocks/library/heading/index.js:26
msgid "Heading %s"
msgstr ""

#: blocks/library/image/index.js:10
msgid "Image"
msgstr ""
Expand Down