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

Text formatting UI #460

Merged
merged 7 commits into from
Apr 24, 2017
Merged

Text formatting UI #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
395ad1b
Text formatting UI
ellatrix Apr 20, 2017
c48a996
Move nodeChange callback to separate function
ellatrix Apr 24, 2017
6546b7c
Remove double
ellatrix Apr 24, 2017
4e00fec
Remove unnecessary logic in and
ellatrix Apr 24, 2017
76e3da6
Fix linting error
ellatrix Apr 24, 2017
6b61f8c
Move formatting control settings to
ellatrix Apr 24, 2017
a8c964d
Optimise in . Props @aduth
ellatrix Apr 24, 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
43 changes: 42 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 @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { last } from 'lodash';
import { forEach, last } from 'lodash';
import { Parser as HtmlToReactParser } from 'html-to-react';

/**
Expand All @@ -11,6 +11,11 @@ import { Parser as HtmlToReactParser } from 'html-to-react';
import './style.scss';

const htmlToReactParser = new HtmlToReactParser();
const formatMap = {
strong: 'bold',
em: 'italic',
del: 'strikethrough'
};

export default class Editable extends wp.element.Component {
constructor() {
Expand All @@ -21,6 +26,8 @@ export default class Editable extends wp.element.Component {
this.onNewBlock = this.onNewBlock.bind( this );
this.bindNode = this.bindNode.bind( this );
this.onFocus = this.onFocus.bind( this );
this.onNodeChange = this.onNodeChange.bind( this );
this.formats = {};
}

componentDidMount() {
Expand Down Expand Up @@ -50,6 +57,10 @@ export default class Editable extends wp.element.Component {
editor.on( 'focusout', this.onChange );
editor.on( 'NewBlock', this.onNewBlock );
editor.on( 'focusin', this.onFocus );

if ( this.props.onFormatChange ) {
editor.on( 'nodechange', this.onNodeChange );
}
}

onInit() {
Expand Down Expand Up @@ -119,6 +130,20 @@ export default class Editable extends wp.element.Component {
} );
}

onNodeChange( { parents } ) {
this.formats = parents.reduce( ( result, node ) => {
const tag = node.nodeName.toLowerCase();

if ( formatMap.hasOwnProperty( tag ) ) {
result[ formatMap[ tag ] ] = true;
}

return result;
}, {} );

this.props.onFormatChange( this.formats );
}

bindNode( ref ) {
this.node = ref;
}
Expand Down Expand Up @@ -183,6 +208,22 @@ export default class Editable extends wp.element.Component {
}
}

componentWillReceiveProps( nextProps ) {
forEach( nextProps.formats, ( state, format ) => {
const currentState = this.formats[ format ] || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we find a way to avoid the cache this.formats but it's ok for now.

Copy link
Member

Choose a reason for hiding this comment

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

Is this.formats here any different than this.props.formats? Like Riad, I also instinctively cringe at the sight of instance variables, though they're sometimes justified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, It can be different than this.props.formats right after we trigger the onFormatChange callback.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions here are welcome.


if ( state !== currentState ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest an early return to avoid tabbing the rest of the callback:

if ( state === currentState ) {
	return;
}

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 how that helps?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how that helps?

It's more a style consideration of avoiding pyramid of doom (and in other cases, callback hell), where the problems become:

  • Line length
  • Maintaining block closings
    • Or in the case of the reader, making sense of separation between if and else blocks, when blocks are particularly long
  • In the case of callbacks, having a good sense of independent function responsibilities

All of which are easily addressed by an emphasis on keeping code as shallow as possible by terminating early or deferring remainder of logic to separate function(s).

In this case, the code is not particularly difficult to make sense of. I only make the suggestion as I find it's almost always a better style to default to, especially if we expect to add more to these functions in the future (becomes less easy or obvious to flatten in the future).

Copy link
Member Author

Choose a reason for hiding this comment

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

It definitely makes sense to make a habit of this. Thanks for elaborating @aduth!

this.editor.focus();

if ( state ) {
this.editor.formatter.apply( format );
} else {
this.editor.formatter.remove( format );
}
}
} );
}

render() {
const { tagName: Tag = 'div', style, className } = this.props;
const classes = classnames( 'blocks-editable', className );
Expand Down
4 changes: 3 additions & 1 deletion blocks/library/text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ registerBlock( 'core/text', {
}
],

edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus } ) {
edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus, onFormatChange, formats } ) {
const { content = <p />, align } = attributes;

return (
Expand All @@ -64,6 +64,8 @@ registerBlock( 'core/text', {
content: after
} ) );
} }
onFormatChange={ onFormatChange }
formats={ formats }
/>
);
},
Expand Down
52 changes: 52 additions & 0 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,62 @@ import Toolbar from 'components/toolbar';
import BlockMover from 'components/block-mover';
import BlockSwitcher from 'components/block-switcher';

const formattingControls = [
{
icon: 'editor-bold',
title: wp.i18n.__( 'Bold' ),
format: 'bold'
},
{
icon: 'editor-italic',
title: wp.i18n.__( 'Italic' ),
format: 'italic'
},
{
icon: 'editor-strikethrough',
title: wp.i18n.__( 'Strikethrough' ),
format: 'strikethrough'
}
];

class VisualEditorBlock extends wp.element.Component {
constructor() {
super( ...arguments );
this.bindBlockNode = this.bindBlockNode.bind( this );
this.setAttributes = this.setAttributes.bind( this );
this.maybeDeselect = this.maybeDeselect.bind( this );
this.maybeHover = this.maybeHover.bind( this );
this.onFormatChange = this.onFormatChange.bind( this );
this.toggleFormat = this.toggleFormat.bind( this );
this.previousOffset = null;
this.state = {
formats: {}
};
}

bindBlockNode( node ) {
this.node = node;
}

onFormatChange( formats ) {
if ( ! this.state.hasEditable ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit like a hack :) but in the same time I can't find a better alternative

Copy link
Member Author

Choose a reason for hiding this comment

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

It does :)

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative would be an extra setting for it, but not sure if that's any better.

this.setState( { hasEditable: true } );
}

this.setState( { formats } );
}

toggleFormat( format ) {
const { formats } = this.state;

this.setState( {
formats: {
...formats,
[ format ]: ! formats[ format ]
}
} );
}

componentWillReceiveProps( newProps ) {
if (
this.props.order !== newProps.order &&
Expand Down Expand Up @@ -119,6 +161,14 @@ class VisualEditorBlock extends wp.element.Component {
isActive: () => control.isActive( block.attributes )
} ) ) } />
) }
{ this.state.hasEditable && (
<Toolbar
controls={ formattingControls.map( ( control ) => ( {
...control,
onClick: () => this.toggleFormat( control.format ),
isActive: () => !! this.state.formats[ control.format ]
} ) ) } />
) }
</div>
}
<BlockEdit
Expand All @@ -127,6 +177,8 @@ class VisualEditorBlock extends wp.element.Component {
setAttributes={ this.setAttributes }
insertBlockAfter={ onInsertAfter }
setFocus={ onFocus }
onFormatChange={ this.onFormatChange }
formats={ this.state.formats }
/>
</div>
);
Expand Down
1 change: 1 addition & 0 deletions editor/modes/visual-editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@

.editor-visual-editor__block-controls .editor-toolbar {
display: inline-flex;
margin-right: 10px;
}

.editor-visual-editor__block-controls .editor-block-switcher {
Expand Down
12 changes: 12 additions & 0 deletions languages/gutenberg.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@ msgstr ""
msgid "Publish"
msgstr ""

#: editor/modes/visual-editor/block.js:17
msgid "Bold"
msgstr ""

#: editor/modes/visual-editor/block.js:22
msgid "Italic"
msgstr ""

#: editor/modes/visual-editor/block.js:27
msgid "Strikethrough"
msgstr ""

#: editor/header/mode-switcher/index.js:24
msgctxt "Name for the Text editor tab (formerly HTML)"
msgid "Text"
Expand Down