Create an Editor component #61

goshacmd · 2016-09-14T10:05:37Z

I was able to get onBlur to work by upgrading Draft.js.

Reading/writing status is reflected by the small status line in the editor:

This change is

goshacmd · 2016-09-14T10:09:16Z

src/components/editor/spec.js

+  let instance = getShallowInstance(<Editor />);
+
+  t.equals(getState(instance.props.children[0]), 'Reading');
+  t.equals(instance.props.children[1].type, UpstreamEditor);


(Correct me if I'm wrong) There does not appear to be properly rendering testing infrastructure in place, which is needed in order to test style-stripping on paste, and the click event which leads to read/write status change. So I wanted to touch base regarding it first.

We should be shallow rendering with enzyme; I thought it was set up on master, but it is not. Just npm i -D enzyme and you're good to go. Sorry for the confusion.

Also, more broadly, I want to avoid rendering into the DOM to test anything. So let's try to find the shallow equivalent before we render all the way out. Feel free to highlight any blocks you might have trouble with and I'll provide suggestions/approaches.

joshdmiller

Overall, this looks pretty darn good. I have a few small things noted here.

Also let me get the storybook up and running today so that check will pass.

joshdmiller · 2016-09-14T21:06:51Z

src/components/outlines/editor/index.js

 import reactStamp from 'react-stamp';
+import EditorFactory from 'components/editor';


I appreciate the effort here, but I should have been more clear: this file is garbage. The outlines section of the site will be discarded before the new editor goes live anywhere, so there's no need to refactor this. If you'd like, I can delete this on master and you can rebase (or you can git rm this directory and the associated route, but I know it wasn't part of the spec for this issue).

But it also appears to be used by characters and elements. Should removal of this file at least come after #58 when we have a component on top of Editor that provides mentions?

The implementation and usage of the editor will change with the new version (such as with data fetching). I have separate issues to be created for migrating to the new editor in other places. To keep the tests passing, let's go ahead and leave this file unchanged, and we'll remove it in another issue.

joshdmiller · 2016-09-14T21:11:27Z

src/components/editor/index.js

+
+  init() {
+    const text = 'Write here...';
+    const content = this.props.content || ContentState.createFromBlockArray([


Per the spec, this component will always be passed a raw content block (the old editor provided its own default). So you can simply convertFromRaw and pass the result directly into createWithContent below. So 47-54 can be replaced with one line.

Sure, changed.

joshdmiller · 2016-09-14T21:13:44Z

src/components/editor/index.js

+
+const EditorComponent = (React, ...behaviours) => reactStamp(React).compose({
+  propTypes: {
+    content: PropTypes.object,


Let's define a simple shape here (the one from the spec should be sufficient). It needs only check the top-level keys, but it will help with documentation/developer mistakes.

joshdmiller · 2016-09-14T21:16:27Z

BTW, that's my first use of GitHub's new review feature, so here's to testing it out! 🍸

goshacmd · 2016-09-15T08:37:10Z

src/components/editor/index.js

+
+  onClick() {
+    this.setState({ readOnly: false }, () => {
+      this.refs.upstream && this.refs.upstream.focus();


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

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.

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

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

goshacmd · 2016-09-15T08:40:23Z

src/components/editor/spec.js

+
+  instance.simulate('click');
+
+  t.equals(getState(instance), 'Writing');


With shallow rendering (and the hack I mentioned in another comment), we can test whether r/w status indicator changes on click.

What we can't test though, is any text area manipulations (typing, pasting, blur) and how the editor responds to them. This looks one of those cases where rendering to DOM can actually make sense. (and require jsdom setup or similar).

I'm not sure to which other comment you're referring.

BUT. Again, I want to draw the distinction between a unit test and an integration test. Typing is handled by Draft, so as far as a unit test is concerned, its functionality is in the trust. All we care about for this component's unit test is this component's functionality. For example, on click, does the editor component get a changed readOnly only prop? And we are supposed to trust that onBlur works as defined, so we don't care for a unit test whether it does - we only care if the callback we pass to that Editor prop does what it's supposed to.

Similarly with pasting. The plugin is a unit, so it's tested separately (see another comment in this review). This component is another unit. So for this component's test, we only need ensure we are passing the plugin downstream - we assume that it works as defined, so we don't test its functionality again here.

But in an integration test, we test that it all works together. We'll be working to add those as "end-to-end" tests (they'll be separate github issues) once we start incorporating these components into routes. In the meantime, we can see if these isolated components perform as expected in integration through storybook stories.

joshdmiller

I only commented on the testing pieces, just to move the conversation in that direction now. I think your focus in testing may be a little off, which is creating more work for you than we're asking. More than happy to discuss this in more detail.

Also, let's add a storybook story, since we finally have that all integrated (see my comments in the #general slack channel).

joshdmiller · 2016-09-16T16:41:15Z

src/components/outlines/editor/index.js

 import reactStamp from 'react-stamp';
+import EditorFactory from 'components/editor';


The implementation and usage of the editor will change with the new version (such as with data fetching). I have separate issues to be created for migrating to the new editor in other places. To keep the tests passing, let's go ahead and leave this file unchanged, and we'll remove it in another issue.

joshdmiller · 2016-09-16T16:42:50Z

src/components/editor/spec.js

+  const content = genContent('Write here');
+  const instance = shallow(<Editor content={content} />);
+
+  t.equals(getState(instance), 'Reading');


Every assertion should include a human-readable description of that's being tested (the final argument to any tape assertion function).

joshdmiller · 2016-09-16T16:50:04Z

src/components/editor/index.js

+}
+
+const stripPastePlugin = {
+  handlePastedText(text, html, { getEditorState, setEditorState }) {


This plugin should have its own unit tests. This is the reason it needed to be a plugin. For in that world, we can have a "strip paste plugin factory". A unit test tests a "unit". In this case, that our stripping is correctly working. We assume all other components are working per spec - including convertFromHTML.

Ergo, if we pass the conversion function into the plugin creation factory, we can just pass in a mock in our test - because we're not trying to test Draft's code. And therefore, the original "pasted" HTML doesn't matter either. We just pass a mock convertFromHTML function that returns a pre-defined content block that we ensure gets appropriately stripped before passing to setEditorState.

wdyt?

joshdmiller · 2016-09-16T16:53:03Z

src/components/editor/index.js

+
+  onClick() {
+    this.setState({ readOnly: false }, () => {
+      this.refs.upstream && this.refs.upstream.focus();


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.

joshdmiller · 2016-09-16T16:59:26Z

src/components/editor/spec.js

+
+  instance.simulate('click');
+
+  t.equals(getState(instance), 'Writing');


I'm not sure to which other comment you're referring.

BUT. Again, I want to draw the distinction between a unit test and an integration test. Typing is handled by Draft, so as far as a unit test is concerned, its functionality is in the trust. All we care about for this component's unit test is this component's functionality. For example, on click, does the editor component get a changed readOnly only prop? And we are supposed to trust that onBlur works as defined, so we don't care for a unit test whether it does - we only care if the callback we pass to that Editor prop does what it's supposed to.

Similarly with pasting. The plugin is a unit, so it's tested separately (see another comment in this review). This component is another unit. So for this component's test, we only need ensure we are passing the plugin downstream - we assume that it works as defined, so we don't test its functionality again here.

But in an integration test, we test that it all works together. We'll be working to add those as "end-to-end" tests (they'll be separate github issues) once we start incorporating these components into routes. In the meantime, we can see if these isolated components perform as expected in integration through storybook stories.

goshacmd · 2016-09-16T21:27:09Z

Done.

sethmerrick had a problem deploying to storybooks.io September 14, 2016 10:05 Error

goshacmd reviewed Sep 14, 2016
View reviewed changes

joshdmiller reviewed Sep 14, 2016

View reviewed changes

goshacmd force-pushed the editor-comp-57 branch from 2b95626 to 6d731d7 Compare September 15, 2016 08:34

sethmerrick had a problem deploying to storybooks.io September 15, 2016 08:34 Error

goshacmd commented Sep 15, 2016

View reviewed changes

goshacmd force-pushed the editor-comp-57 branch from 6d731d7 to 209bf09 Compare September 16, 2016 06:53

sethmerrick deployed to storybooks.io September 16, 2016 06:53 View deployment

goshacmd force-pushed the editor-comp-57 branch from 209bf09 to 201a05b Compare September 16, 2016 07:02

sethmerrick deployed to storybooks.io September 16, 2016 07:02 View deployment

joshdmiller requested changes Sep 16, 2016

View reviewed changes

goshacmd force-pushed the editor-comp-57 branch from 201a05b to e608a89 Compare September 16, 2016 21:25

sethmerrick deployed to storybooks.io September 16, 2016 21:25 View deployment

sethmerrick deployed to storybooks.io September 22, 2016 07:08 View deployment

sethmerrick approved these changes Sep 22, 2016

View reviewed changes

joshdmiller approved these changes Sep 22, 2016

View reviewed changes

goshacmd mentioned this pull request Sep 24, 2016

Implement the ReferenceEditor component #120

Open

goshacmd and others added 2 commits September 27, 2016 14:49

Create an Editor component. Ref GH-57

c654214

refactor: revised editor unit tests, implementation

45bf05f

joshdmiller force-pushed the editor-comp-57 branch from c2a0b14 to 45bf05f Compare September 27, 2016 21:49

sethmerrick deployed to storybooks.io September 27, 2016 21:49 View deployment

joshdmiller merged commit 45bf05f into master Sep 27, 2016

joshdmiller mentioned this pull request Sep 27, 2016

editor component #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an Editor component #61

Create an Editor component #61

goshacmd commented Sep 14, 2016 •

edited by joshdmiller

Loading

goshacmd Sep 14, 2016

joshdmiller Sep 14, 2016

joshdmiller left a comment

joshdmiller Sep 14, 2016

goshacmd Sep 15, 2016

joshdmiller Sep 16, 2016

joshdmiller Sep 14, 2016 •

edited

Loading

goshacmd Sep 15, 2016

joshdmiller Sep 14, 2016

joshdmiller commented Sep 14, 2016

goshacmd Sep 15, 2016

joshdmiller Sep 16, 2016

goshacmd Sep 16, 2016

joshdmiller Sep 16, 2016

goshacmd Sep 15, 2016

joshdmiller Sep 16, 2016 •

edited

Loading

joshdmiller left a comment

joshdmiller Sep 16, 2016

joshdmiller Sep 16, 2016

joshdmiller Sep 16, 2016

joshdmiller Sep 16, 2016

joshdmiller Sep 16, 2016 •

edited

Loading

goshacmd commented Sep 16, 2016

		import reactStamp from 'react-stamp';
		import EditorFactory from 'components/editor';


		instance.simulate('click');

		t.equals(getState(instance), 'Writing');

Create an Editor component #61

Create an Editor component #61

Conversation

goshacmd commented Sep 14, 2016 • edited by joshdmiller Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdmiller Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdmiller commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdmiller Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

joshdmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdmiller Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

goshacmd commented Sep 16, 2016

goshacmd commented Sep 14, 2016 •

edited by joshdmiller

Loading

joshdmiller Sep 14, 2016 •

edited

Loading

joshdmiller Sep 16, 2016 •

edited

Loading

joshdmiller Sep 16, 2016 •

edited

Loading