Implement the ReferenceEditor component #120

goshacmd · 2016-09-24T09:41:56Z

Ref GH-58

The PR is based on GH-61, which wasn't merged yet, so the diff view might be a bit less useful.

Suggestion list is the default one, not material. The reason for this is the fact that the mentions plugins allows to supply a custom class name for the suggestion list, but not a custom component. material-ui obviously does not export the class names, as there are no class names to begin with.
The current testing setup breaks when importing a component, due to its includes of stylesheets, so I haven't pushed the test. I recall you guys acknowledged the issue.

This change is

joshdmiller · 2016-09-26T19:49:07Z

I'm a little confused on a couple of things. Perhaps you can clarify.

ComfyEditor component

Where did that name come from? I am not following the logic of the naming scheme.

Suggestion list is the default one, not material. The reason for this is the fact that the mentions plugins allows to supply a custom class name for the suggestion list, but not a custom component.

Looking at the docs, it looks like the props mentionComponent and entryComponent do what we want. Would those not work?

The current testing setup breaks when importing a component...I recall you guys acknowledged the issue.

I don't remember the discussion, but what exactly is the issue? Is the dependent component from Draft Plugins requiring CSS is its files? Or where are the stylesheets being included that are causing the problem?

goshacmd · 2016-09-26T20:29:41Z

Where did that name come from? I am not following the logic of the naming scheme.

As in, a comfortable and properly set up editor. Happy to change whatever you deem appropriate. The name of the component wasn't specified, and Editor2 wouldn't be better.

Looking at the docs, it looks like the props mentionComponent and entryComponent do what we want. Would those not work?

It's true that you kinda can specify an entry component in the mention list. You can't, however, make the MentionSuggestions into a proper List, it will still stay a div.

Another issue is that that ListItem is visually marked as hovered only if a) you mouse-hover it, or b) tab-navigate into it, without an option to mark it as such from props. Arrowing between list items then would be either impossible or involve some amount of hackery, such as programmatically emitting the tab navigation event in case an item has isFocused === true prop.

Or where are the stylesheets being included that are causing the problem?

Yep, seems like autoprefixer issue; it needs browser environment to detect what to prefix. Obviously there is none.

joshdmiller · 2016-09-26T20:42:24Z

As in, a comfortable and properly set up editor. Happy to change whatever you deem appropriate. The name of the component wasn't specified, and Editor2 wouldn't be better.

I guess since the issue was "reference editor component", I assumed it would be called that. We will be using the normal Editor in some circumstances and the ReferenceEditor (the one that has been augmented to allow referencing characters and elements) in others.

It's true that you kinda can specify an entry component in the mention list. You can't, however, make the MentionSuggestions into a proper List, it will still stay a div.

Another issue is that that ListItem is visually marked as hovered only if a) you mouse-hover it, or b) tab-navigate into it, without an option to mark it as such from props. Arrowing between list items then would be either impossible or involve some amount of hackery

The default looks very out of place in the material world, and I am not seeing any visual marking of either the hover or the keyboard focus in the Storybook. The spec was for the Material-UI components, so that's what I would like to see, please.

seems like autoprefixer issue;

Can you be more specific? Where is the issue occurring?

goshacmd · 2016-09-26T21:01:36Z

Re naming my bad, "reference" read like a verb there.

Changed the list styling to drop the shadow and render individual items as material.

> [email protected] test:unit /Users/goshakkk/Projects/org/storyshop/app
> tape build/specs.js | faucet

webpack:///../~/style-loader/addStyles.js?:14
        return /msie [6-9]\b/.test(window.navigator.userAgent.toLowerCase());
                                   ^

ReferenceError: window is not defined
    at eval (webpack:///../~/style-loader/addStyles.js?:14:30)
    at eval (webpack:///../~/style-loader/addStyles.js?:9:47)
    at module.exports (webpack:///../~/style-loader/addStyles.js?:31:68)
    at eval (webpack:///../~/draft-js-mention-plugin/lib/plugin.css?:7:39)
    at Object.<anonymous> (/Users/goshakkk/Projects/org/storyshop/app/build/specs.js:11141:2)
    at __webpack_require__ (/Users/goshakkk/Projects/org/storyshop/app/build/specs.js:20:30)
    at eval (webpack:///./components/reference-editor/index.js?:35:1)
    at Object.<anonymous> (/Users/goshakkk/Projects/org/storyshop/app/build/specs.js:10487:2)
    at __webpack_require__ (/Users/goshakkk/Projects/org/storyshop/app/build/specs.js:20:30)
    at eval (webpack:///./components/reference-editor/spec.js?:19:9)

joshdmiller · 2016-09-26T21:04:02Z

Re naming my bad, "reference" read like a verb there.

Ah! That makes much more sense now. I didn't notice the ambicategoricality. #wordoftheday 😄

joshdmiller · 2016-09-26T21:06:34Z

Well, the material components fit better, but you make a good point about they keyboard navigation. They are also aggressively large. However, I think we can live with it for an MVP. cc @sethmerrick

sethmerrick · 2016-09-26T21:15:41Z

@joshdmiller I agree this will work for MVP, and also agree it leaves room to iterate into a solution that matches Material spec and offers keyboard navigation.

joshdmiller · 2016-09-26T21:17:31Z

@goshakkk Check out my new PR: #126. I think it should help with your unit testing.

goshacmd · 2016-09-27T07:07:53Z

Hm, after rebasing on master, it looks like the css is wrapped into js for some reason when running tests:

/Users/goshakkk/Projects/org/storyshop/app/node_modules/draft-js-mention-plugin/lib/plugin.css:1
(function (exports, require, module, __filename, __dirname) { .draftJsMentionPlugin__mention__29BEd, .draftJsMentionPlugin__mention__29BEd:visited {

joshdmiller · 2016-09-27T07:15:52Z

The change I made isn't on master yet.

goshacmd · 2016-09-27T07:19:58Z

@joshdmiller I know, and I cherry-picked it separately. The window issue doesn't seem to be related and something else seems to be happening instead.

joshdmiller · 2016-09-27T22:02:28Z

@goshakkk: What's the status of this PR?

goshacmd · 2016-09-28T06:35:34Z

@joshdmiller that tests CSS issue is the only blocker

goshacmd · 2016-09-28T18:07:06Z

@joshdmiller ok, ready for review now.

joshdmiller · 2016-09-28T18:45:26Z

@goshakkk Can you rebase against master? Some of those commits (like the base Editor) are already in there (as of yesterday, I think) and it will make the diff easier to browse. Thanks!

joshdmiller · 2016-09-30T04:43:18Z

src/components/reference-editor/mention-text.js

+import { cyan700, green700 } from 'material-ui/lib/styles/colors';
+
+const MentionComponent = ({ mention, className, mentionPrefix, children }) => (
+  <a


Should be a react router Link.

joshdmiller · 2016-09-30T04:43:39Z

src/components/reference-editor/index.js

+import MentionComponent from './mention-text';
+import SuggestionsFactory from './suggestions';
+
+export default function ({


Why is this a factory?

To allow injecting plugins, to test

I am a little concerned with this approach. This makes the reference editor export a "factory factory" (i.e. a factory that returns a factory that returns a react component). And we must have all invocations throughout the application call a function that, as far as the calling code is concerned, doesn't do anything, just to make use of the component. And that's all just to make testing a little easier.

Can we approach this without the factory factory?

Two other options would be:

have private methods on the component that will return the instances of plugins and UpstreamEditor, then either expect those or stub — but this seems a bit too involved

export the actual instances of plugins and UpstreamEditor, and set expectations based on those

I've chosen to go the second road and pushed just now.

goshacmd · 2016-10-01T13:15:09Z

@joshdmiller any updates?

joshdmiller · 2016-10-05T21:32:10Z

src/components/reference-editor/index.js

+import MentionComponent from './mention-text';
+import SuggestionsFactory from './suggestions';
+
+const Editor = EditorFactory(React);


The purpose of passing React as an argument to the factory is to ensure the same instance is used everywhere. When done properly, there isn't the need to import React at all. Let's move the call of this factory to inside the factory beneath so as to take advantage of that same instance.

Oops, right!

joshdmiller · 2016-10-05T21:34:13Z

src/components/reference-editor/spec.js

+import { shallow, mount } from 'enzyme';
+import spy, { createSpy } from '../../utils/spy';
+
+import {


Doesn't look like most of these are used.

Yeah... I feel like we could benefit from an eslint rule to detect unused identifiers

joshdmiller · 2016-10-05T21:35:50Z

src/components/reference-editor/spec.js

+  }
+
+  {
+    const upstream = instance.find(UpstreamEditor).at(0);


If we're not creating a new instance, I don't think we need to re-find this.

joshdmiller · 2016-10-05T21:38:50Z

src/components/reference-editor/spec.js

+  {
+    const upstream = instance.find(UpstreamEditor).at(0);
+    const plugins = upstream.props().plugins;
+    t.notEquals(plugins.indexOf(mentionPlugin), -1, 'should manage mention plugin');


From looking at these tests, it seems the whole discussion about the "factory factory" might be related to just these three lines. If so, in the future, I don't feel this is something we would necessarily need to test. To some degree, it's just testing for a typo. If we wanted to be extra sure, we could check that the number of plugins passed matches our expectation, but I don't think we got a lot of value out of this particular set of assertions.

It's fine to leave, since the work is already done, but for the future, I consider stuff like this superfluous, so you need't worry about testing it too much.

joshdmiller · 2016-10-05T21:39:46Z

src/components/reference-editor/story.js

+
+const Editor = EditorFactory(React);
+
+import {


Doesn't look like we need all of these.

joshdmiller · 2016-10-05T21:44:32Z

src/components/reference-editor/suggestions/index.js

+
+export default function (MentionSuggestions) {
+  return ({ suggestions, ...rest }) => (
+    <MentionSuggestions


Would it help with styling if we wrapped this in a List?

Not really, looks like there isn't a lot of styling in List itself and ListItems are just enough

goshacmd · 2016-10-06T16:40:44Z

Just pushed some updates

joshdmiller · 2016-10-06T19:19:41Z

Looks good. Let's just add the commit footer and call it done!

goshacmd · 2016-10-12T18:51:06Z

@joshdmiller which commit footer? Is there any action required on my part?

sethmerrick · 2016-10-12T18:58:26Z

@goshakkk I believe @joshdmiller is requesting a new line at the end of the commit message to close the associated issue like Closes #58.

You can see more specific details about the footer in https://github.com/StoryShop/app/blob/master/CONTRIBUTING.md#commit-messages and let me know if you have any questions. Thanks!

Closes GH-58

goshacmd · 2016-10-12T19:01:59Z

Done

sethmerrick deployed to storybooks.io September 24, 2016 09:41 View deployment

goshacmd force-pushed the comfy-editor-58 branch from 061dd1c to fec13ba Compare September 26, 2016 20:59

sethmerrick deployed to storybooks.io September 26, 2016 20:59 View deployment

goshacmd changed the title ~~Implement the ComfyEditor component~~ Implement the ReferenceEditor component Sep 26, 2016

goshacmd force-pushed the comfy-editor-58 branch from fec13ba to 8fc11be Compare September 28, 2016 18:06

sethmerrick deployed to storybooks.io September 28, 2016 18:06 View deployment

chore: add fake user agent for unit tests

d331b78

goshacmd force-pushed the comfy-editor-58 branch from 8fc11be to e0aa770 Compare September 28, 2016 18:49

sethmerrick deployed to storybooks.io September 28, 2016 18:49 View deployment

joshdmiller reviewed Sep 30, 2016

View reviewed changes

Ignore css requires in tests

12cf55c

goshacmd force-pushed the comfy-editor-58 branch from e0aa770 to 1c55f3a Compare September 30, 2016 07:33

sethmerrick deployed to storybooks.io September 30, 2016 07:33 View deployment

goshacmd self-assigned this Oct 1, 2016

goshacmd force-pushed the comfy-editor-58 branch from 1c55f3a to 5aac21f Compare October 5, 2016 08:37

sethmerrick requested a deployment to storybooks.io October 5, 2016 08:37 Abandoned

joshdmiller reviewed Oct 5, 2016

View reviewed changes

goshacmd force-pushed the comfy-editor-58 branch from 5aac21f to b9cb062 Compare October 6, 2016 16:39

sethmerrick requested a deployment to storybooks.io October 6, 2016 16:39 Abandoned

joshdmiller approved these changes Oct 6, 2016

View reviewed changes

feat: Implement the ReferenceEditor component

94606f6

Closes GH-58

goshacmd force-pushed the comfy-editor-58 branch from b9cb062 to 94606f6 Compare October 12, 2016 19:01

sethmerrick requested a deployment to storybooks.io October 12, 2016 19:01 Abandoned

Implement the ReferenceEditor component #120

Are you sure you want to change the base?

Implement the ReferenceEditor component #120

Conversation

goshacmd commented Sep 24, 2016 • edited by joshdmiller Loading

joshdmiller commented Sep 26, 2016

goshacmd commented Sep 26, 2016

joshdmiller commented Sep 26, 2016

goshacmd commented Sep 26, 2016

joshdmiller commented Sep 26, 2016

joshdmiller commented Sep 26, 2016

sethmerrick commented Sep 26, 2016

joshdmiller commented Sep 26, 2016

goshacmd commented Sep 27, 2016 • edited Loading

joshdmiller commented Sep 27, 2016

goshacmd commented Sep 27, 2016

joshdmiller commented Sep 27, 2016

goshacmd commented Sep 28, 2016

goshacmd commented Sep 28, 2016

joshdmiller commented Sep 28, 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

Choose a reason for hiding this comment

goshacmd commented Oct 1, 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

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

goshacmd commented Oct 6, 2016

joshdmiller commented Oct 6, 2016

goshacmd commented Oct 12, 2016

sethmerrick commented Oct 12, 2016

goshacmd commented Oct 12, 2016

goshacmd commented Sep 24, 2016 •

edited by joshdmiller

Loading

goshacmd commented Sep 27, 2016 •

edited

Loading