Skip to content

Editable: implement TinyMCE on the core Editable component #330

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

Merged
youknowriad merged 8 commits into from
Mar 28, 2017
Merged

Editable: implement TinyMCE on the core Editable component #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d7c9bb7
TinyMCE: V1 of the Editable Component initializing TinyMCE
youknowriad Mar 27, 2017
4b7a51a
Changes per review
youknowriad Mar 28, 2017
a6bfbcf
Mock Component to fix unit tests
youknowriad Mar 28, 2017
cf04a33
Drop the global contenteditable
youknowriad Mar 28, 2017
f80239f
Remove the `isFocused` check
youknowriad Mar 28, 2017
97ba143
Expose package globals in test build
aduth Mar 28, 2017
84f31be
Rename TinyMCE dependency to clarify nightly
aduth Mar 28, 2017
b33ed3a
Adding native browser spellcheck
youknowriad Mar 28, 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
3 changes: 2 additions & 1 deletion .eslintrc.json
View file
Open in desktop
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
76 changes: 74 additions & 2 deletions blocks/components/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,75 @@
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.bindNode = this.bindNode.bind( this );
}

componentDidMount() {
this.initialize();
}

initialize() {
const config = {
Copy link
Member

@ellatrix ellatrix Mar 28, 2017

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,
browser_spellcheck: true,
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( 'focusout', this.onChange );
Copy link

@spocke spocke Mar 28, 2017

Choose a reason for hiding this comment

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

focusout will when focus is shifted internally within nested contenteditables. Use "blur" instead or possible 'change' since that is called on every undo level add including 'blur' since that adds an undo level. Might be to frequent though.

Copy link
Contributor Author

@youknowriad youknowriad Mar 29, 2017

Choose a reason for hiding this comment

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

Since this is a "controlled" TinyMCE (each time the content prop changes, we update the content), I had issues while using change on the per-block prototype. I haven't tried blur though

}

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 );
}

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

updateContent() {
const bookmark = this.editor.selection.getBookmark( 2, true );
this.editor.setContent( this.props.value );
this.editor.selection.moveToBookmark( bookmark );
}

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

@spocke spocke Mar 28, 2017

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 } />;
}
}
4 changes: 2 additions & 2 deletions editor/blocks/text-block/index.js
View file
Open in desktop
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

@aduth aduth Mar 27, 2017

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

@youknowriad youknowriad Mar 28, 2017

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.

Copy link

@joyously joyously Mar 28, 2017

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

@youknowriad youknowriad Mar 28, 2017

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)

Copy link

@joyously joyously Mar 28, 2017

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

@youknowriad youknowriad Mar 28, 2017

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...

Copy link

@joyously joyously Mar 28, 2017

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

@youknowriad youknowriad Mar 28, 2017

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
2 changes: 1 addition & 1 deletion editor/editor/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import InserterButton from '../inserter/button';
const Editor = ( { state: { blocks, inserter }, toggleInserter } ) => {
return (
<div>
<div contentEditable>
<div>
{ blocks.map( ( block, index ) =>
<div key={ index }>
{ wp.blocks.getBlockSettings( block.blockType ).edit( block.attributes ) }
Expand Down
17 changes: 9 additions & 8 deletions element/index.js
View file
Open in desktop
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
View file
Open in desktop
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-nightly', '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-nightly' ) );
}
add_action( 'init', 'gutenberg_register_scripts' );

Expand Down
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"eslint-config-wordpress": "^1.1.0",
"eslint-plugin-jsx-a11y": "^4.0.0",
"eslint-plugin-react": "^6.10.3",
"expose-loader": "^0.7.3",
"extract-text-webpack-plugin": "^2.1.0",
"glob": "^7.1.1",
"jsdom": "^9.12.0",
Expand All @@ -42,6 +43,8 @@
"pegjs-loader": "^0.5.1",
"postcss-loader": "^1.3.3",
"raw-loader": "^0.5.1",
"react": "^15.4.2",
"react-dom": "^15.4.2",
"sass-loader": "^6.0.3",
"sinon": "^2.1.0",
"sinon-chai": "^2.9.0",
Expand Down
14 changes: 13 additions & 1 deletion webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,19 @@ switch ( process.env.NODE_ENV ) {

case 'test':
config.target = 'node';
config.entry = glob.sync( `./{${ Object.keys( config.entry ).join() }}/test/*.js` );
config.module.rules = [
...config.module.rules,
...[ 'element', 'blocks', 'editor' ].map( ( entry ) => ( {
test: require.resolve( './' + entry + '/index.js' ),
use: 'expose-loader?wp.' + entry
} ) )
];
config.entry = [
'./element/index.js',
'./blocks/index.js',
'./editor/index.js',
...glob.sync( `./{${ Object.keys( config.entry ).join() }}/test/*.js` )
];
config.externals = [ require( 'webpack-node-externals' )() ];
config.output = {
filename: 'build/test.js',
Expand Down