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

Create an Editor component #61

Merged
merged 2 commits into from
Sep 27, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 7 additions & 0 deletions conf/storybook/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"presets": [
"es2015",
"react",
"stage-0"
]
}
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
},
"homepage": "https://github.com/StoryShop/app#readme",
"devDependencies": {
"@kadira/storybook": "^2.14.0",
"@kadira/storybook": "^2.5.2",
"autoprefixer": "^6.3.3",
"babel-loader": "^6.2.2",
"babel-plugin-module-resolver": "^2.2.0",
Expand All @@ -60,9 +60,9 @@
"babel-runtime": "^6.5.0",
"css-loader": "^0.23.1",
"debug": "^2.2.0",
"draft-js": "^0.5.0",
"draft-js-mention-plugin": "^1.1.0",
"draft-js-plugins-editor": "^1.0.1",
"draft-js": "0.9.0",
"draft-js-mention-plugin": "1.1.0",
"draft-js-plugins-editor": "1.1.0",
"enzyme": "^2.4.1",
"express": "^4.13.4",
"falcor": "^0.1.16",
Expand Down
115 changes: 115 additions & 0 deletions src/components/editor/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import { PropTypes } from 'react';
import reactStamp from 'react-stamp';
import { fromJS, List, Repeat } from 'immutable';
import Editor from 'draft-js-plugins-editor';
import {
EditorState,
ContentBlock,
convertFromRaw,
RichUtils,
BlockMapBuilder,
genKey,
CharacterMetadata,
} from 'draft-js';

import stripPastePluginFactory from './paste-plugin';

const stripPastePlugin = stripPastePluginFactory();

const EditorComponent = (React, ...behaviours) => reactStamp(React).compose({
propTypes: {
content: PropTypes.shape({
entityMap: PropTypes.object.isRequired,
blocks: PropTypes.array.isRequired,
}).isRequired,

onEditStart: PropTypes.func,
onEditEnd: PropTypes.func,
onChange: PropTypes.func,

readOnly: PropTypes.bool,
plugins: PropTypes.array,
},

init() {
this.state = {
editor: EditorState.createWithContent( convertFromRaw( this.props.content ) ),
readOnly: true,
};
},

componentWillUnmount() {
this.deleteTimeout();
this.save();
},

onClick() {
this.setState({ readOnly: false }, () => {
this.refs.upstream && this.refs.upstream.focus();
Copy link
Member Author

Choose a reason for hiding this comment

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

Shallow rendering does not support refs, therefore we can't always place focus on click.

Choose a reason for hiding this comment

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

As per another comment, we assume Draft's focus function works as expected for the purpose of a unit test. If we want to check our onClick does the correct behaviour, we really only want to check that it sets state and calls the function. This is arguably a case where a unit test is the wrong solution. One could make the case that this is more appropriate for an integration test.

However, one way to make this testable is to have the second argument for setState be ::this._focus, which contains the same logic. Then in our unit test, we set the component instance method instance._focus to a spy and ensure the spy gets called.

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 would argue unit testing is barely making sense in this particular case. The purpose of unit testing is test behavior of a particular unit, but the scope of unit testing is concerned only with public API. _focus is anything but public API, as its implementation and even the underscore-prefixed name suggest. The only public API of React components is its rendered data.

It does pass the feel-good check of having everything tested, but I want to ask, what's the value of this test? Like, really.

The primary value of testing lies in automation of manual testing process, which is both tedious and time consuming. The most natural thing to test, so far as many things concerning the UI, is what component renders and how it reacts to the user interactions — by literally encoding in code how the user would interact and how the rendered content would change as the result. Which is literally what integration tests do.

Unit tests, on another hand, are most valuable when you have a clear input -> output, like making sure add(2, 2) produces 4, and so on.

It's a bit of a mystery to me as to why you insist on having all of that unit-tested, instead of integration-tested which clearly seems to make more sense.

I can make all the proposed changes if only to move this PR forward as it's already taking more time than I imagined it would

Choose a reason for hiding this comment

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

Perhaps I wasn't clear, because I think you misunderstood my comments.

The purpose of unit testing is test behavior of a particular unit, but the scope of unit testing is concerned only with public API. _focus is anything but public API, as its implementation and even the underscore-prefixed name suggest. The only public API of React components is its rendered data.

Right. That was entire reason I suggested moving the logic to an underscore-prefixed function. I suggested moving the logic to a place where we knowingly would not test it, but that if we wanted to test the surrounding functionality, which is part of the public API, we could do so by factoring it out as I outlined above: again, intentionally avoiding testing _focus.

It's a bit of a mystery to me as to why you insist on having all of that unit-tested, instead of integration-tested which clearly seems to make more sense.

This entire conversation, from github to slack and back was because you said you needed a DOM to unit test this component. I said you didn't, and we've been going back and forth on that fine point ever since. That was probably myopic of us.

Setting that aside, I direct you to my actual comment: "This is arguably a case where a unit test is the wrong solution". So I am not insisting that everything be unit-tested. I honestly don't know where you've gotten that impression; I don't remember ever having a conversation about my test coverage requirements. In this case, I was simply responding to your comment about the refs.


Ok, let's take a step back. I know this has taken far longer than either of us have expected (and we'll work through that part together, offline), but I feel part of that is because we keep going in circles. Here's how I see us getting to the finish line here.

  • this PR should reflect the issue spec as defined; the spec (including its implementation requirements) is not negotiable once work has started (feedback, discussion, and suggestions are encouraged and appreciated before work starts, though!);
  • this PR should include a few storybook stories to demonstrate functionality visually; they are required for all React components (except when explicitly stated);
  • this PR should include whatever unit tests you feel are required, knowing that shallow rendering is all that is presently available to you;

At that point, I can do a proper review.

Hit me up with any questions. 👍

});
},

_focus() {
this.refs.upstream.focus();
},

onFocus() {
if (this.props.onEditStart) {
this.props.onEditStart();
}
},

onBlur() {
this.setState({ readOnly: true });
if (this.props.onEditEnd) {
this.props.onEditEnd();
}
},

onChange(editor) {
if (this.props.readOnly) {
return;
}

this.setState({ editor });

if (this.props.onChange) {
this.deleteTimeout();
this._timeout = setTimeout(::this.save, 3000);
}
},

deleteTimeout() {
if (this._timeout) {
clearTimeout(this._timeout);
this._timeout = null;
}
},

save() {
if (this.props.onChange) {
this.props.onChange(this.state.editor.getCurrentContent());
}
},

render() {
const { plugins = [] } = this.props;
const isReadOnly = this.props.readOnly ? true : this.state.readOnly;

return (
<div onClick={::this.onClick}>
<Editor
ref="upstream"
editorState={this.state.editor}
plugins={[ ...plugins, stripPastePlugin ]}
onChange={::this.onChange}
onFocus={::this.onFocus}
onBlur={::this.onBlur}
readOnly={isReadOnly}
/>
</div>
);
},
});

export default EditorComponent;
45 changes: 45 additions & 0 deletions src/components/editor/paste-plugin/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import {
EditorState,
convertFromHTML,
Modifier,
BlockMapBuilder,
} from 'draft-js';

const stripPastePlugin = ( convert = convertFromHTML ) => ({
handlePastedText(text, html, { getEditorState, setEditorState }) {
if ( ! html ) {
return;
}

const htmlFrag = convert( html );
if ( ! htmlFrag ) {
return;
}

const ALLOWED_STYLE = ['ITALIC', 'BOLD'];
const contentBlocks = htmlFrag.map(block => {
const characterList = block.getCharacterList().map(list => {
const styles = list.getStyle().filter(s => {
return ALLOWED_STYLE.indexOf(s) !== -1;
});
return list.set('style', styles);
});
return block.set('type', 'unstyled').set('characterList', characterList);
});

const htmlMap = BlockMapBuilder.createFromArray(contentBlocks);

const editorState = getEditorState();
const newContent = Modifier.replaceWithFragment(
editorState.getCurrentContent(),
editorState.getSelection(),
htmlMap
);

setEditorState( EditorState.push( editorState, newContent, 'insert-fragment' ) );

return true;
},
});

export default stripPastePlugin;
106 changes: 106 additions & 0 deletions src/components/editor/paste-plugin/spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import test from 'tape';
import spy, { createSpy } from '../../../utils/spy';

import {
EditorState,
ContentState,
ContentBlock,
CharacterMetadata,
convertToRaw,
convertFromRaw,
genKey,
} from 'draft-js';
import { is, fromJS, List, Repeat, Record } from 'immutable';

import stripPastePluginFactory from './';

test( 'StripPastePlugin', t => {
let plugin, actual, expected, result, convertSpy;

const html = '<p>does not matter</p>';

const initalContentState = ContentState.createFromBlockArray([
new ContentBlock({
key: 'initial',
type: 'unstyled',
text: 'Initial',
characterList: List( Repeat( CharacterMetadata.EMPTY, 7 ) ),
}),
]);
const initialEditorState = EditorState.createWithContent( initalContentState );

const mocks = {
setEditorState: () => {},
getEditorState: () => initialEditorState,
};
const setEditorStateSpy = spy( mocks, 'setEditorState' );
const getEditorStateSpy = spy( mocks, 'getEditorState' );

convertSpy = createSpy( () => [
new ContentBlock({
key: 'bold',
type: 'unstyled',
text: 'bold',
characterList: List( Repeat( CharacterMetadata.applyStyle( CharacterMetadata.EMPTY, 'BOLD' ), 4 ) ),
}),
new ContentBlock({
key: 'header',
type: 'header-two',
text: 'header',
characterList: List( Repeat( CharacterMetadata.EMPTY, 6 ) ),
}),
]);

plugin = stripPastePluginFactory( convertSpy );

/**
* Ignore Pasted Text
*/
{
result = plugin.handlePastedText( null, null, mocks );
t.notOk( result, 'should return false when no html is provided' );
}

/**
* Process Pasted HTML
*/
{
result = plugin.handlePastedText( null, html, mocks );
t.ok( result, 'should return true' );
t.equal( convertSpy.calls.length, 1, 'should call convert' );
t.equal( convertSpy.calls[ 0 ].args[ 0 ], html, 'should call convert with the html' );
t.equal( setEditorStateSpy.calls.length, 1, 'should call setEditorState' );
}

/**
* Strip Pasted HTML
*/
{
let blocks = setEditorStateSpy.calls[ 0 ].args[ 0 ].getCurrentContent().getBlockMap();

actual = blocks.count();
expected = 2;
t.equal( actual, expected, 'should result in two blocks' );

let firstBlock = blocks.first();
let secondBlock = blocks.last();

actual = firstBlock.get( 'text' );
expected = 'bold';
t.equals( actual, expected, 'first block should be first block of pasted text' );

actual = firstBlock.getInlineStyleAt( 0 ).toJS();
expected = [ 'BOLD' ];
t.deepEquals( actual, expected, 'pasted bold text should remain bold' );

actual = secondBlock.get( 'text' );
expected = 'headerInitial';
t.equals( actual, expected, 'second block should combine pasted with intial content' );

actual = secondBlock.get( 'type' );
expected = 'unstyled';
t.equals( actual, expected, 'second block should be unstyled' );
}

t.end();
});
88 changes: 88 additions & 0 deletions src/components/editor/spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import React from 'react';
import test from 'tape';
import { shallow, mount } from 'enzyme';
import spy, { createSpy } from '../../utils/spy';

import {
EditorState,
ContentState,
ContentBlock,
CharacterMetadata,
convertToRaw,
convertFromRaw,
genKey,
} from 'draft-js';

import EditorFactory from './';
import UpstreamEditor from 'draft-js-plugins-editor';

const Editor = EditorFactory(React);

test('Editor', t => {
let instance, actual, expected, editor;

const content = {
entityMap: {},
blocks: [{ text: '' }],
};

instance = shallow( <Editor content={content} /> );

/**
* Default state
*/
{
let editor = instance.find( UpstreamEditor ).at( 0 );
t.ok( editor, 'should render a Draft Plugins Editor' );
t.ok( editor.props().readOnly, 'should start in readOnly mode' );
}

/**
* Writable on click
*/
{
instance.simulate( 'click' );

let editor = instance.find( UpstreamEditor ).at( 0 );
t.notOk( editor.props().readOnly, 'should move to writable mode on click' );
}

/**
* Read only
*/
{
instance = shallow( <Editor content={content} readOnly={true} /> );
instance.simulate( 'click' );

let editor = instance.find( UpstreamEditor ).at( 0 );
t.ok( editor.props().readOnly, 'should not move to writeable on click if read only' );
}

/**
* Lifecycle methods
*/
{
let onEditStartSpy = createSpy();
let onEditEndSpy = createSpy();

instance = shallow(
<Editor
content={content}
onEditStart={onEditStartSpy}
onEditEnd={onEditEndSpy}
/>
);

t.equals( onEditStartSpy.calls.length, 0, 'onEditStart is not called initially');
t.equals( onEditEndSpy.calls.length, 0, 'onEditEnd is not called initially');

instance.instance().onFocus();
t.equals(onEditStartSpy.calls.length, 1, 'onEditStart is called after focusing in');

instance.instance().onBlur();
t.equals(onEditEndSpy.calls.length, 1, 'onEditEnd is called after blurring out');
}

t.end();
});

Loading