feat(Comment): Add component #584

layershifter · 2016-10-02T08:33:13Z

Fixes #187.

layershifter · 2016-10-02T08:34:56Z

@levithomason I added first doc example, LGTM.

codecov-io · 2016-10-02T08:38:02Z

Current coverage is 100% (diff: 100%)

Merging #584 into master will not change coverage

@@           master   #584   diff @@
====================================
  Files         119    128     +9   
  Lines        1890   1962    +72   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1890   1962    +72   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 3fb0ca7...5b904ea

levithomason · 2016-10-03T16:12:55Z

docs/app/Examples/views/Comment/Content/CommentExampleReplyForm.js

+        <Comment.Actions>
+          <Comment.Action active>Reply</Comment.Action>
+        </Comment.Actions>
+        <Comment.ReplyForm>


I see the reply class is required for the form when used in a comment or after all comments. I think we should explore some other API options for adding the class.

This pattern reminds me of the label and button class being required for Inputs that are "labeled` or have an "action". In the Input case the label/action is passed through props, so we have an opportunity to add the className without requiring additional components or props to add the class.

Ideas:

Form reply prop

We could simply add a reply prop to the Form. It would clean this up and clarify things. However, this prop is not applicable anywhere except in comments, so it seems sub optimal.

Comment replyForm prop

I think this one makes more sense since you'll want the same form in all comments. Rather than pasting a reply form in every comment and one at the end of all comments, we could create a single comment reply form and pass it as a prop to each comment:

const replyForm = <Form /> <Comment.Group replyForm={replyForm} /> // and/or <Comment replyForm={replyForm} />

Looping Comments

This brings me to another realization, I think the actual React usage of the Comment will be to create a single comment component, query a server for a list of comments, then loop over the comments mapping the single comment component. Because of this, I don't think most users will ever build out a comment thread in a hard coded sense.

That would put the Form reply prop back on the table since there would likely only be one comment component in your project (reply form included) that you would map over your comment data.

Feedback?

We have same situation with List.Icon, if we add reply prop on Form, seems we'll need to remove List.Icon.

@jcarbo ?

I'm really torn on this one. While I agree entirely on the need for consistency with List.Icon, at the same time I really want to avoid this API:

<Comment.ReplyForm> <Form.TextArea /> <Button labeled icon primary> <Icon name='edit' /> Add Reply </Button> </Comment.ReplyForm>

I think it comes off very confusing IMHO. It would be great to get a single top level interface to creating a Comment form some how.

In this way, reply prop looks more reliable solution.

…nologyAdvice/stardust into feat/comment Conflicts: src/views/index.js

layershifter · 2016-10-04T17:54:24Z

I've finished docs, tomorrow I'll add tests and finalize component.

What is the decision for Comment.ReplyForm? We add reply prop to form?

levithomason · 2016-10-04T20:56:39Z

Let's go with the reply prop on Form for now. We can revisit later if necessary.

…nologyAdvice/stardust into feat/comment Conflicts: README.md

layershifter · 2016-10-05T15:44:39Z

Added test, seems we need review there 👍
Pay attention to CommentAvatar, not sure that there is a good solution.

Also, there are no shorthands, I think we can do them in sepatate PR.

levithomason · 2016-10-05T16:22:43Z

Agreed on separate PR for shorthand, I'll get a review here soon as I can.

levithomason

See comment on CommentExampleThreaded.js, will finish review after that.

levithomason · 2016-10-05T16:30:18Z

docs/app/Examples/views/Comment/Variations/CommentExampleThreaded.js

+      </Comment.Content>
+    </Comment>
+
+    <Comment.ReplyForm>


This component no longer exists, docs fail to load at present:

Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components). Check the render method of `CommentExampleThreaded`. CommentExampleThreaded.js?42c9:180

Stupid miss, will fix soon.

layershifter · 2016-10-05T17:55:25Z

Docs again with us.

levithomason

Only a few comments, then I think we can merge this guy.

levithomason · 2016-10-05T19:02:41Z

docs/app/Examples/views/Comment/Content/CommentExampleReplyForm.js

+        <Comment.Actions>
+          <Comment.Action active>Reply</Comment.Action>
+        </Comment.Actions>
+        <Form reply>


To prevent page refresh on submit, let's add:

onSubmit={e => e.preventDefault()}

levithomason · 2016-10-05T19:03:27Z

docs/app/Examples/views/Comment/Content/index.js

+
+    <ComponentExample
+      title='Reply Form'
+      description='A comment can contain a form to reply to a comment. This may have arbitrary content'


Needs a . "This may have arbitrary content."

Stupid question. Why we need add trailing dot there, but don't add it in other places? 🤔

No, it's a great question :) We need to get consistent. Currently, when there is a single sentence, the period is sometimes left out. I can see doing this as it is trying to keep it minimal and feeling like a heading.

When there are multiple sentences, the last sentence needs a period. Otherwise, there is one terminated sentence, and another not.

Because of all this, I propose we just always include the period. So it is consistent and we don't have issues with multi-sentence descriptions like this one.

LGTM, I'll add trailing dot to all example descriptions tomorrow, revolutionary improvement :)

levithomason · 2016-10-05T19:06:59Z

docs/app/Examples/views/Comment/Content/index.js

+      title='Reply Form'
+      description='A comment can contain a form to reply to a comment. This may have arbitrary content'
+      examplePath='views/Comment/Content/CommentExampleReplyForm'
+    />


Suggest we also add this info Message from the SUI docs:

If a comment form is located inside a comment it will be formatted as a nested reply form. If the comment form is included after all comments, it will be formatted as a normal reply form.

levithomason · 2016-10-05T19:14:57Z

docs/app/Examples/views/Comment/States/CommentExampleCollapsed.js

+import React, { Component } from 'react'
+import { Checkbox, Comment } from 'semantic-ui-react'
+
+export default class CommentExampleCollapsed extends Component {


This is a cool example, really like the simple interactivity to show how a prop works.

…nologyAdvice/stardust into feat/comment

layershifter · 2016-10-06T05:55:03Z

Updated to master, fixed review comments.

layershifter · 2016-10-06T18:00:35Z

Hm... @levithomason can you restart build? Strange failure there.

levithomason · 2016-10-06T19:29:14Z

Intermittent failures associated with the Search tests, I've seen this a couple times now:

  Search Component
    active item
      ✖ moves up on arrow up when open
        PhantomJS 2.1.1 (Linux 0.0.0)
        expected the node in <Search /> to have a 'active' propHTML: Not available due to: null is not an object (evaluating 'this.el.outerHTML')

I restarted the build.

I believe since you have access to the repo, you should also have access to the builds at https://circleci.com/gh/Semantic-Org/Semantic-UI-React.

layershifter · 2016-10-07T14:10:03Z

Added dots 😆 Seems, we ready to go

levithomason · 2016-10-08T04:28:23Z

Another one down, thanks!

levithomason · 2016-10-08T04:37:31Z

Released in [email protected]

feat(Comment): Add component

bf38cf6

layershifter mentioned this pull request Oct 2, 2016

feat(Comment): add component #392

Closed

feat(Comment): More docs

36810fd

layershifter added the enhancement label Oct 2, 2016

feat(Comment): More docs

16c35dc

levithomason reviewed Oct 3, 2016

View reviewed changes

Alexander Fedyashov added 2 commits October 4, 2016 18:19

Merge branches 'feat/comment' and 'master' of https://github.com/Tech…

b891ba6

…nologyAdvice/stardust into feat/comment Conflicts: src/views/index.js

feat(Comment): More docs

bf65e8f

Merge branches 'feat/comment' and 'master' of https://github.com/Tech…

b244f14

…nologyAdvice/stardust into feat/comment Conflicts: README.md

layershifter mentioned this pull request Oct 5, 2016

chore(warnings): Fix warnings in Form and Modal #626

Merged

Alexander Fedyashov added 2 commits October 5, 2016 18:03

fix(Comment): Replace ReplyForm, add tests to Form

b3764b2

feat(Comment): Add tests

e4df1f8

layershifter added the ready for review label Oct 5, 2016

levithomason requested changes Oct 5, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Oct 5, 2016

fix(Comment): fix docs

9e96531

layershifter added ready for review and removed needs author feedback labels Oct 5, 2016

levithomason requested changes Oct 5, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Oct 5, 2016

Merge branches 'feat/comment' and 'master' of https://github.com/Tech…

90cc23b

…nologyAdvice/stardust into feat/comment

feat(Comment): Fix review comments

133d46a

layershifter added ready for review and removed needs author feedback labels Oct 6, 2016

docs(Comment): add trailing dots

5b904ea

levithomason merged commit 4d6b7e3 into master Oct 8, 2016

levithomason deleted the feat/comment branch October 8, 2016 04:28

levithomason mentioned this pull request Oct 12, 2016

docs(Radio): refactor, use stateless functional expression and fix a typo #662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Comment): Add component #584

feat(Comment): Add component #584

layershifter commented Oct 2, 2016 •

edited

Loading

layershifter commented Oct 2, 2016 •

edited

Loading

codecov-io commented Oct 2, 2016 •

edited

Loading

levithomason Oct 3, 2016 •

edited

Loading

layershifter Oct 3, 2016

levithomason Oct 3, 2016 •

edited

Loading

layershifter Oct 4, 2016

layershifter commented Oct 4, 2016

levithomason commented Oct 4, 2016

layershifter commented Oct 5, 2016 •

edited

Loading

levithomason commented Oct 5, 2016

levithomason left a comment

levithomason Oct 5, 2016

layershifter Oct 5, 2016

layershifter commented Oct 5, 2016

levithomason left a comment

levithomason Oct 5, 2016

levithomason Oct 5, 2016

layershifter Oct 6, 2016 •

edited

Loading

levithomason Oct 6, 2016 •

edited

Loading

layershifter Oct 6, 2016

levithomason Oct 5, 2016 •

edited

Loading

levithomason Oct 5, 2016

layershifter commented Oct 6, 2016

layershifter commented Oct 6, 2016

levithomason commented Oct 6, 2016 •

edited

Loading

layershifter commented Oct 7, 2016

levithomason commented Oct 8, 2016

levithomason commented Oct 8, 2016

feat(Comment): Add component #584

feat(Comment): Add component #584

Conversation

layershifter commented Oct 2, 2016 • edited Loading

layershifter commented Oct 2, 2016 • edited Loading

codecov-io commented Oct 2, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

levithomason Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Form reply prop

Comment replyForm prop

Looping Comments

Choose a reason for hiding this comment

levithomason Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Oct 4, 2016

levithomason commented Oct 4, 2016

layershifter commented Oct 5, 2016 • edited Loading

levithomason commented Oct 5, 2016

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Oct 5, 2016

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Oct 6, 2016

layershifter commented Oct 6, 2016

levithomason commented Oct 6, 2016 • edited Loading

layershifter commented Oct 7, 2016

levithomason commented Oct 8, 2016

levithomason commented Oct 8, 2016

layershifter commented Oct 2, 2016 •

edited

Loading

layershifter commented Oct 2, 2016 •

edited

Loading

codecov-io commented Oct 2, 2016 •

edited

Loading

levithomason Oct 3, 2016 •

edited

Loading

Form `reply` prop

Comment `replyForm` prop

levithomason Oct 3, 2016 •

edited

Loading

layershifter commented Oct 5, 2016 •

edited

Loading

layershifter Oct 6, 2016 •

edited

Loading

levithomason Oct 6, 2016 •

edited

Loading

levithomason Oct 5, 2016 •

edited

Loading

levithomason commented Oct 6, 2016 •

edited

Loading