Implement the ReferenceEditor component

goshacmd commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

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

> storyshop-app@1.0.0 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 commented 8 years ago

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

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

joshdmiller commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

goshacmd commented 8 years ago

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 commented 8 years ago

The change I made isn't on master yet.

goshacmd commented 8 years ago

@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 commented 8 years ago

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

goshacmd commented 8 years ago

@joshdmiller that tests CSS issue is the only blocker

goshacmd commented 8 years ago

@joshdmiller ok, ready for review now.

joshdmiller commented 8 years ago

@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!

goshacmd commented 8 years ago

@joshdmiller any updates?

goshacmd commented 8 years ago

Just pushed some updates

joshdmiller commented 8 years ago

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

goshacmd commented 8 years ago

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

sethmerrick commented 8 years ago

@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!

goshacmd commented 8 years ago

Done

StoryShop / app

Implement the ReferenceEditor component #120