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.

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: implement TinyMCE on the core Editable component #330

Merged
merged 8 commits into from
Mar 28, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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 .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
}
},
"globals": {
"wp": true
"wp": true,
"tinymce": true
},
"plugins": [
"react",
Expand Down
92 changes: 90 additions & 2 deletions blocks/components/editable/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,91 @@
export default function Editable( { value } ) {
return <p>{ value }</p>;
export default class Editable extends wp.element.Component {
constructor() {
super( ...arguments );
this.onInit = this.onInit.bind( this );
this.onSetup = this.onSetup.bind( this );
this.onChange = this.onChange.bind( this );
this.onFocusIn = this.onFocusIn.bind( this );
this.onFocusOut = this.onFocusOut.bind( this );
this.bindNode = this.bindNode.bind( this );
}

componentDidMount() {
this.initialize();
}

initialize() {
const config = {
Copy link
Member

Choose a reason for hiding this comment

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

To recreate default contenteditable behaviour, we should probably add browser_spellcheck: true.

target: this.node,
theme: false,
inline: true,
toolbar: false,
entity_encoding: 'raw',
setup: this.onSetup,
formats: {
strikethrough: { inline: 'del' }
}
};

tinymce.init( config );
}

onSetup( editor ) {
this.editor = editor;
editor.on( 'init', this.onInit );
editor.on( 'focusIn', this.onFocusIn );
editor.on( 'focusout', this.onFocusOut );
}

onInit() {
this.editor.setContent( this.props.value );
}

onChange() {
if ( ! this.editor.isDirty() ) {
return;
}
const value = this.editor.getContent();
this.editor.save();
this.props.onChange( value );
}

onFocusIn() {
this.isFocused = true;
}

onFocusOut() {
this.isFocused = false;
this.onChange();
}

bindNode( ref ) {
this.node = ref;
}

updateContent() {
let bookmark;
if ( this.isFocused ) {
bookmark = this.editor.selection.getBookmark( 2, true );
}
this.editor.setContent( this.props.value );
if ( this.isFocused ) {
this.editor.selection.moveToBookmark( bookmark );
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, does it make any difference if we were to call this while the editor does not have focus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can cause the cursor to jump to the beginning of the content editable. I think right now, it does not happen because we're not updating the content from outside the component yet.
I've had some hard time fixing these kind of bugs on the per-block prototype.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah my thinking was if the editor wasn't focused anyways, setting content and moving bookmark wouldn't have any impact on the current active focus. I did some basic testing and this appears to be the case? I was entering this in my console at page load:

var bookmark = tinymce.editors[ 1 ].selection.getBookmark( 2, true );
tinymce.editors[ 1 ].setContent( tinymce.editors[ 0 ].getContent() );
tinymce.editors[ 1 ].selection.moveToBookmark( bookmark );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Can't remember exactly why I had those on the per-block editor. Removing these checks for now

}
}

componentWillUnmount() {
if ( this.editor ) {
this.editor.destroy();
Copy link

Choose a reason for hiding this comment

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

Use this.editor.remove(); destroy is more for memory cleanups on unload of the page not really a problem now days on modern browsers. Destroy is called by remove.

}
}

componentDidUpdate( prevProps ) {
if ( this.props.value !== prevProps.value ) {
this.updateContent();
}
}

render() {
return <div ref={ this.bindNode } />;
}
}
5 changes: 5 additions & 0 deletions bootstrap-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@ global.document = require( 'jsdom' ).jsdom( '', {
} );
global.window = document.defaultView;
global.navigator = window.navigator;
global.wp = {
Copy link
Member

Choose a reason for hiding this comment

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

The issue seems to be that the globals are only defined in the main bundle configuration, whereas the entry point of the tests bundle includes only test files. I might toy with the configuration a bit to see if I can fix it so we don't need to stub these out.

element: {
Component: function() {}
}
};
4 changes: 2 additions & 2 deletions editor/blocks/text-block/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
const { query, html } = wp.blocks.query;
const { html } = wp.blocks.query;
const Editable = wp.blocks.Editable;

wp.blocks.registerBlock( 'core/text', {
title: 'Text',
icon: 'text',

attributes: {
value: query( 'p', html() )
value: html()
Copy link
Member

Choose a reason for hiding this comment

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

Might still be value in specifying 'p' argument to at least limit value to text within the <p> element of the block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. If we limit the value to the text within the p, we should limit the Editable to create a unique p and the onChange callback of the Editable component should extract the content of the p before calling the onChange prop.

This is probably what we want, I was thinking we could leave this to a separate PR.

Choose a reason for hiding this comment

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

How does specifying p here affect wpautop() type of stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is unrelated to wpautop.

The wpautop needs to be implemented by catching the Enter key press and checking whether we should create a new block after the current one or not. (Similar behaviour can be seen on the per-block prototype)

Choose a reason for hiding this comment

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

Well, sort of. What I was interested in was what is actually saved. If I look at the post content of my current blog, there are a lot of text paragraphs separated by one blank line. There are not actual p tags. Somewhere along the way to display wpautop puts p tags? Will the new editor save the p tag that the old editor did not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming we're talking about the visual editor. Then yes, in its current form this block saves the content with the p tag. A block declares its saved markup in the save function. You can see that the current wp/text block wraps the content with a p tag.

But this is far from being the final version of the text block, it's just here to serve as an example of block while we're building the "surroundings", how we parse the content, how we save it back, how we render the editables etc...

Choose a reason for hiding this comment

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

Yes, it's an example, so I would hope you "do it right". If it saves the p to the content (where the old editor did not), does that affect wpautop?
I get that "it's not the final version" answer a lot, but my experience is that those things are then forgotten and harder to change later. I'm just trying to think of consequences down the line, instead of "building this block now".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the exact reason why we're trying to build this as a plugin and to tie it to real data (real posts), for now, my answer to this is "I don't know". We'll have to try the editor in the context of real data to figure out these consequences (and fix all the unwanted behaviour).

},

edit( attributes, onChange ) {
Expand Down
17 changes: 9 additions & 8 deletions element/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* External dependencies
*/
import { createElement as reactCreateElement } from 'react';
import { render as reactRender } from 'react-dom';
import { createElement, Component } from 'react';
import { render } from 'react-dom';

/**
* Returns a new element of given type. Type can be either a string tag name or
Expand All @@ -15,16 +15,17 @@ import { render as reactRender } from 'react-dom';
* @param {...wp.Element} children Descendant elements
* @return {wp.Element} Element
*/
export function createElement( type, props, ...children ) {
return reactCreateElement( type, props, ...children );
}
export { createElement };

/**
* Renders a given element into the target DOM node.
*
* @param {wp.Element} element Element to render
* @param {Element} target DOM node into which element should be rendered
*/
export function render( element, target ) {
reactRender( element, target );
}
export { render };

/**
* A base class to create WordPress Components (Refs, state and lifecycle hooks)
*/
export { Component };
3 changes: 2 additions & 1 deletion index.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ function gutenberg_register_scripts() {
wp_register_script( 'react-dom', 'https://unpkg.com/react-dom@15/dist/react-dom' . $suffix . '.js', array( 'react' ) );

// Editor
wp_register_script( 'tinymce_js', 'https://fiddle.azurewebsites.net/tinymce/nightly/tinymce.min.js' );
wp_register_script( 'wp-element', plugins_url( 'element/build/index.js', __FILE__ ), array( 'react', 'react-dom' ) );
wp_register_script( 'wp-blocks', plugins_url( 'blocks/build/index.js', __FILE__ ), array( 'wp-element' ) );
wp_register_script( 'wp-blocks', plugins_url( 'blocks/build/index.js', __FILE__ ), array( 'wp-element', 'tinymce_js' ) );
}
add_action( 'init', 'gutenberg_register_scripts' );

Expand Down