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

Editable: Adding an inline link control to the format toolbar #524

Merged
merged 6 commits into from
May 2, 2017
Merged

Editable: Adding an inline link control to the format toolbar #524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28b7e06
Editable: Adding an inline link control to the format toolbar
youknowriad Apr 27, 2017
d6a006c
Format Toolbar: Small style changes
youknowriad Apr 27, 2017
609f2a5
Link Control: Adding the view link state
youknowriad Apr 28, 2017
328b212
Link control: Trigger the edit state when inserting an new link
youknowriad Apr 28, 2017
31a5778
Format Controls: Fixing the empty link insertion
youknowriad May 1, 2017
910d2dc
Format Controls: Leverage the Toolbar component
youknowriad May 1, 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
159 changes: 159 additions & 0 deletions blocks/components/editable/format-toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/**
* Internal dependencies
*/
// TODO: We mustn't import by relative path traversing from blocks to editor
// as we're doing here; instead, we should consider a common components path.
import IconButton from '../../../editor/components/icon-button';
import Toolbar from '../../../editor/components/toolbar';

const FORMATTING_CONTROLS = [
{
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 FormatToolbar extends wp.element.Component {
constructor( props ) {
super( ...arguments );
this.state = {
linkValue: props.formats.link ? props.formats.link.value : '',
isEditingLink: false
};
this.addLink = this.addLink.bind( this );
this.editLink = this.editLink.bind( this );
this.dropLink = this.dropLink.bind( this );
this.submitLink = this.submitLink.bind( this );
this.updateLinkValue = this.updateLinkValue.bind( this );
}

componentWillUnmout() {
if ( this.editTimeout ) {
clearTimeout( this.editTimeout );
}
}

componentWillReceiveProps( nextProps ) {
const newState = {
linkValue: nextProps.formats.link ? nextProps.formats.link.value : ''
};
if (
! this.props.formats.link ||
! nextProps.formats.link ||
this.props.formats.link.node !== nextProps.formats.link.node
) {
newState.isEditingLink = false;
}
this.setState( newState );
}

toggleFormat( format ) {
return () => {
this.props.onChange( {
[ format ]: ! this.props.formats[ format ]
} );
};
}

addLink() {
if ( ! this.props.formats.link ) {
this.props.onChange( { link: { value: '' } } );

// Debounce the call to avoid the reset in willReceiveProps
this.editTimeout = setTimeout( () => this.setState( { isEditingLink: true } ) );
}
}

dropLink() {
this.props.onChange( { link: undefined } );
}

editLink( event ) {
event.preventDefault();
this.setState( {
isEditingLink: true
} );
}

submitLink( event ) {
event.preventDefault();
this.props.onChange( { link: { value: this.state.linkValue } } );
this.setState( {
isEditingLink: false
} );
}

updateLinkValue( event ) {
this.setState( {
linkValue: event.target.value
} );
}

render() {
const { formats, focusPosition } = this.props;
const linkStyle = focusPosition
? { position: 'absolute', ...focusPosition }
: null;

return (
<div className="editable-format-toolbar">
<Toolbar
controls={
FORMATTING_CONTROLS
.map( ( control ) => ( {
...control,
onClick: this.toggleFormat( control.format ),
isActive: !! formats[ control.format ]
} ) )
.concat( [ {
Copy link
Member

Choose a reason for hiding this comment

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

If we're not using FORMATTING_CONTROLS for anything else, and we're always concatenating, why not just include the link control in the constant set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't have the same isActive and onClick prop. Do you prefer a ternary or something?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I overlooked how we're mapping to add in instance-bound handlers. This should be fine as-is.

icon: 'admin-links',
title: wp.i18n.__( 'Link' ),
onClick: this.addLink,
isActive: !! formats.link
} ] )
}
/>

{ !! formats.link && this.state.isEditingLink &&
<form
className="editable-format-toolbar__link-modal"
style={ linkStyle }
onSubmit={ this.submitLink }>
<input
className="editable-format-toolbar__link-input"
type="url"
required
value={ this.state.linkValue }
onChange={ this.updateLinkValue }
placeholder={ wp.i18n.__( 'Paste URL or type' ) }
/>
<IconButton icon="editor-break" type="submit" />
</form>
}

{ !! formats.link && ! this.state.isEditingLink &&
<div className="editable-format-toolbar__link-modal" style={ linkStyle }>
<a className="editable-format-toolbar__link-value" href="" onClick={ this.editLink }>
{ decodeURI( this.state.linkValue ) }
</a>
<IconButton icon="edit" onClick={ this.editLink } />
<IconButton icon="editor-unlink" onClick={ this.dropLink } />
</div>
}
</div>
);
}
}

export default FormatToolbar;
95 changes: 52 additions & 43 deletions blocks/components/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@
* External dependencies
*/
import classnames from 'classnames';
import { last, isEqual, capitalize, omitBy } from 'lodash';
import { last, isEqual, capitalize, omitBy, forEach, merge } from 'lodash';
import { nodeListToReact } from 'dom-react';
import { Fill } from 'react-slot-fill';
import 'element-closest';

/**
* Internal dependencies
*/
import './style.scss';

import FormatToolbar from './format-toolbar';
// TODO: We mustn't import by relative path traversing from blocks to editor
// as we're doing here; instead, we should consider a common components path.
import Toolbar from '../../../editor/components/toolbar';
Expand All @@ -22,24 +23,6 @@ const formatMap = {
del: 'strikethrough'
};

const FORMATTING_CONTROLS = [
{
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'
}
];

const ALIGNMENT_CONTROLS = [
{
icon: 'editor-alignleft',
Expand Down Expand Up @@ -86,9 +69,11 @@ export default class Editable extends wp.element.Component {
this.onFocus = this.onFocus.bind( this );
this.onNodeChange = this.onNodeChange.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
this.changeFormats = this.changeFormats.bind( this );
this.state = {
formats: {},
alignment: null
alignment: null,
bookmark: null
};
}

Expand All @@ -104,6 +89,7 @@ export default class Editable extends wp.element.Component {
toolbar: false,
browser_spellcheck: true,
entity_encoding: 'raw',
convert_urls: false,
setup: this.onSetup,
formats: {
strikethrough: { inline: 'del' }
Expand Down Expand Up @@ -147,6 +133,15 @@ export default class Editable extends wp.element.Component {
this.props.onChange( this.savedContent );
}

getRelativePosition( node ) {
const editorPosition = this.editorNode.closest( '.editor-visual-editor__block' ).getBoundingClientRect();
const position = node.getBoundingClientRect();
return {
top: position.top - editorPosition.top + 40 + ( position.height ),
left: position.left - editorPosition.left - 157
};
}

isStartOfEditor() {
const range = this.editor.selection.getRng();
if ( range.startOffset !== 0 || ! range.collapsed ) {
Expand Down Expand Up @@ -225,28 +220,26 @@ export default class Editable extends wp.element.Component {
} );
}

onNodeChange( { parents } ) {
onNodeChange( { element, parents } ) {
let alignment = null;
const formats = {};

parents.forEach( ( node ) => {
const tag = node.nodeName.toLowerCase();

if ( formatMap.hasOwnProperty( tag ) ) {
formats[ formatMap[ tag ] ] = true;
} else if ( tag === 'a' ) {
formats.link = { value: node.getAttribute( 'href' ), node };
}

if ( tag === 'p' ) {
alignment = node.style.textAlign || 'left';
}
} );

if (
this.state.alignment !== alignment ||
! isEqual( this.state.formats, formats )
) {
this.setState( { alignment, formats } );
}
const focusPosition = this.getRelativePosition( element );
const bookmark = this.editor.selection.getBookmark( 2, true );
this.setState( { alignment, bookmark, formats, focusPosition } );
}

bindEditorNode( ref ) {
Expand All @@ -260,6 +253,7 @@ export default class Editable extends wp.element.Component {
this.editor.selection.moveToBookmark( bookmark );
// Saving the editor on updates avoid unecessary onChanges calls
// These calls can make the focus jump

this.editor.save();
}

Expand Down Expand Up @@ -326,14 +320,35 @@ export default class Editable extends wp.element.Component {
return !! this.state.formats[ format ];
}

toggleFormat( format ) {
this.editor.focus();

if ( this.isFormatActive( format ) ) {
this.editor.formatter.remove( format );
} else {
this.editor.formatter.apply( format );
changeFormats( formats ) {
if ( this.state.bookmark ) {
this.editor.selection.moveToBookmark( this.state.bookmark );
}

forEach( formats, ( formatValue, format ) => {
if ( format === 'link' ) {
if ( formatValue !== undefined ) {
const anchor = this.editor.dom.getParent( this.editor.selection.getNode(), 'a' );
if ( ! anchor ) {
this.editor.formatter.remove( 'link' );
}
this.editor.formatter.apply( 'link', { href: formatValue.value }, anchor );
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 occur if ! anchor condition above passes and the link is removed? Seems like this ought to be:

if ( anchor ) {
	// apply link
} else {
	// remove link
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not the desired behaviour here.

Actually I'm not sure this is very important but I just mimic the insertLink command here. I think this might be useful if we try to add multiple links to the same node.

} else {
this.editor.execCommand( 'Unlink' );
}
} else {
const isActive = this.isFormatActive( format );
if ( isActive && ! formatValue ) {
this.editor.formatter.remove( format );
} else if ( ! isActive && formatValue ) {
this.editor.formatter.apply( format );
}
}
} );

this.setState( {
formats: merge( {}, this.state.formats, formats )
} );
}

isAlignmentActive( align ) {
Expand Down Expand Up @@ -373,13 +388,7 @@ export default class Editable extends wp.element.Component {
isActive: this.isAlignmentActive( control.align )
} ) ) } />
}

<Toolbar
controls={ FORMATTING_CONTROLS.map( ( control ) => ( {
...control,
onClick: () => this.toggleFormat( control.format ),
isActive: this.isFormatActive( control.format )
} ) ) } />
<FormatToolbar focusPosition={ this.state.focusPosition } formats={ this.state.formats } onChange={ this.changeFormats } />
Copy link
Member

Choose a reason for hiding this comment

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

Might want to split these props onto multiple lines to avoid the long line length.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of calculating the position, would it be possible to do some absolute positioning within a relative control parent for the overlay?

Something like: https://codepen.io/aduth/pen/LyxJEb

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now we're trying to position the overlay atop the selected text. I suppose this works too, just maybe a little unexpected to have to move cursor to toolbar to add link, only to have to move it back to text to configure the link. I do see this is how it works in the current editor though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something for @jasmussen to weigh in on with regards to the placement of the link dialog. Should it be next to the selected text, or next to the now-active link toolbar control?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like this link design (though the specific buttons are a bit quickly put together):

links

@iseulde should be in the loop on the link work we're doing here, because IMO links work better in the current WordPress editor than anywhere else, and she worked a lot on them. Having the dialog under as opposed to over was for a reason, though I can't recall which.

Also, here's an admittedly suboptimal design for when you are linking inside captions:

screen shot 2017-04-27 at 20 41 49

Still thinking about that one, but it's at least one argument for having links under.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we put in under the link because we kept in mind that we might want to do something different above. There might be more reasons I don't remember though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now we're trying to position the overlay atop the selected text. I suppose this works too, just maybe a little unexpected to have to move cursor to toolbar to add link, only to have to move it back to text to configure the link. I do see this is how it works in the current editor though.

We were planning to look into putting the formatting tools inline which would solve this. If we keep them on top, it might be worth exploring other solutions though. There's also cmd+K and pasting links over words :)

</Fill>,
element
];
Expand Down
41 changes: 41 additions & 0 deletions blocks/components/editable/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,44 @@ figcaption.blocks-editable {
font-size: $default-font-size;
}
}


.editable-format-toolbar {
display: inline-flex;
}

.editable-format-toolbar__link-modal {
position: absolute;
top: 42px;
box-shadow: 0px 3px 20px rgba( 18, 24, 30, .1 ), 0px 1px 3px rgba( 18, 24, 30, .1 );
border: 1px solid #e0e5e9;
background: #fff;
width: 250px;
display: inline-flex;
align-items: center;
font-family: $default-font;
font-size: $default-font-size;
line-height: $default-line-height;
}


input.editable-format-toolbar__link-input {
padding: 10px;
font-size: 13px;
width: 100%;
border: none;
outline: none;
box-shadow: none;
flex-grow: 1;

&:focus {
border: none;
box-shadow: none;
outline: none;
}
}

.editable-format-toolbar__link-value {
padding: 10px;
flex-grow: 1;
}
Loading