Comment Styling #2510

NetOpWibby · 2019-05-24T15:10:01Z

PR Checklist

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I have checked that this PR does not introduce a breaking change

PR Type

What kind of change does this PR introduce?

Feature
Code style update (formatting)

Fixes

What is the current behavior?

Comments are not styled.

What is the new behavior?

Comments are styled.

neb-b

Tested and it seems to work pretty good.

neb-b · 2019-06-20T02:10:04Z

src/ui/component/commentCreate/view.jsx

+  const { createComment, claim } = props;
+  const { claim_id: claimId } = claim;
+  const [commentValue, setCommentValue] = usePersistedState(`comment-${claimId}`, '');
+  const [commentAck, setCommentAck] = usePersistedState(COMMENT_ACKNOWLEDGED, 'no');


Instead of 'no', this can just be a boolean

neb-b · 2019-06-20T02:11:31Z

src/ui/component/commentCreate/view.jsx

+            <div className="media__title">About comments..</div>
+          </div>
+          <div className="card__content">
+            <div className="media__subtitle">Seriously, don&apos;t comment.</div>


I'm not sure if we need a title along with this? I guess it depends on what that messaging ends up being.

@kauffj was going to do some copy for this section.

neb-b · 2019-06-20T02:19:49Z

src/ui/component/commentsList/view.jsx

+  }
+}
+
+class Comment extends React.PureComponent<CommentProps> {


Comment should be broken into it's own file. Then in CommentsList, pass commentId that can be used to select the comment.

Then if we ever require more data to display a comment, we only have to update one place, instead of having to pass another prop to it in this file

neb-b · 2019-06-20T02:20:43Z

src/ui/component/fileDetails/view.jsx

@@ -31,14 +31,14 @@ class FileDetails extends PureComponent<Props> {
    Lbryio.call('user_tag', 'edit', { add: 'comments-waitlist' });
    showSnackBar(
      <span>
-        {__('Thanks! Comments are coming soon')}
+        {__('Your Comment Has Been Posted')}


This whole thing can just be removed. We don't need to show a snackbar

neb-b · 2019-06-20T02:21:24Z

src/ui/constants/settings.js

@@ -17,3 +17,5 @@ export const AUTOPLAY = 'autoplay';
 export const RESULT_COUNT = 'resultCount';
 export const OS_NOTIFICATIONS_ENABLED = 'osNotificationsEnabled';
 export const AUTO_DOWNLOAD = 'autoDownload';
+export const COMMENT_ACKNOWLEDGED = 'comment_acknowledged';
+export const COMMENT_ACKNOWLEDGED_TRUE = 'comment_acknowledged_true';


We don't need this

neb-b · 2019-06-20T02:21:44Z

src/ui/page/file/view.jsx

+              <h2 className="card__header">Comments</h2>
+            </header>
+            <CommentCreate uri={uri} />
+            <CommentsList uri={uri} />


neb-b · 2019-06-20T02:22:36Z

src/ui/redux/selectors/app.js

@@ -28,6 +28,7 @@ export const selectUpdateUrl = createSelector(
  }
 );

+// HERE


osilkin98 · 2019-06-24T06:09:50Z

src/ui/scss/component/_comments.scss

+  width: 0; height: 0;
+  transition: border-color 0.2s;
+
+  &.downvote {


We're not doing upvotes/downvotes. The only time I can see that is if when move to using claims as comments, we can count supports with say, 10 confirmations as being upvotes.

kauffj

Some suggested changes, but nothing strong.

This review was inspection only until issues introduced by rebase are addressed.

kauffj · 2019-06-26T16:55:25Z

src/ui/component/commentCreate/view.jsx

+};
+
+export function CommentCreate(props: Props) {
+  const COMMENT_ACKNOWLEDGED = 'COMMENT_ACKNOWLEDGED';


I am all for consting all special values, but this one could probably be skipped

kauffj · 2019-06-26T16:57:00Z

src/ui/component/commentCreate/view.jsx

+
+  return (
+    <React.Fragment>
+      {commentAck !== true && (


if commentAck is false it looks like this returns nothing, so this could probably be tidied down to one check

if it's false, it shows you the acknowledgment box.
if it's true, it shows you the commenting tool.

kauffj · 2019-06-26T17:00:20Z

src/ui/component/commentCreate/view.jsx

+    setCommentAck(true);
+  }
+  function handleSubmit() {
+    if (channel !== 'new' && commentValue.length) createComment(commentValue, claimId, channel);


The new here is a value that comes from <ChannelSection>, which is the right time to use a const (otherwise, when editing ChannelSection, I'd have to check every place it is embedded and manually read all of that code to see if "new" is being used or checked against).

kauffj · 2019-06-26T17:01:35Z

src/ui/component/commentsList/view.jsx

+    return (
+      <li className={this.props.parentId ? 'comment reply' : 'comment'}>
+        <div className="comment__meta card__actions--between">
+          {this.props.author && <strong>{this.props.author}</strong>}


<strong>{this.props.author ? this.props.author : __("Anonymous") }</strong>

(note i18n call as well as simplification)

Use this

{this.props.author}

in place of

<Button className="button--uri-indicator" navigate={this.props.uri}> {inner} </Button> ); }

The schema you're using is a little out of date, you may want to compare it with the one I have on my branch

src/ui/component/commentsList/view.jsx

kauffj · 2019-06-26T17:03:23Z

src/ui/redux/actions/app.js

@@ -373,7 +373,7 @@ export function doChangeVolume(volume) {
    });
  };
 }
-
+// HERE


src/ui/redux/reducers/app.js

kauffj · 2019-06-26T17:04:06Z

src/ui/scss/component/_comments.scss

+.comment {
+  padding-bottom: var(--spacing-vertical-large);
+
+  &.reply {


this should be the class comment--reply and be a first-class rule (i.e. not nested) if we're following BEM

src/ui/scss/component/_comments.scss

kauffj · 2019-06-26T17:07:34Z

src/ui/component/fileViewer/view.jsx

@@ -76,7 +76,7 @@ class FileViewer extends React.PureComponent<Props> {
    }

    this.handleAutoplay(this.props);
-    window.addEventListener('keydown', this.handleKeyDown);
+    // window.addEventListener('keydown', this.handleKeyDown);


is this functionality being removed? maybe comment out the function too and explain why if it's anticipated that it returns?

@jessopb I'm fixing most of my own comments but I don't know what or why this was changed, so I'm leaving as is

lbry-bot assigned NetOpWibby May 24, 2019

jessopb force-pushed the redux-comments branch 2 times, most recently from 9e6909e to da5c17d Compare June 17, 2019 01:29

jessopb mentioned this pull request Jun 17, 2019

Comments #2443

Closed

jessopb assigned jessopb and unassigned NetOpWibby Jun 17, 2019

kauffj requested a review from neb-b June 17, 2019 19:13

lbry-bot assigned neb-b Jun 17, 2019

kauffj requested review from kauffj and neb-b and removed request for neb-b June 17, 2019 19:13

lbry-bot assigned kauffj and neb-b and unassigned neb-b Jun 17, 2019

neb-b suggested changes Jun 20, 2019

View reviewed changes

lbry-bot unassigned neb-b and jessopb Jun 20, 2019

osilkin98 reviewed Jun 24, 2019

View reviewed changes

osilkin98 added a commit to osilkin98/lbry-desktop that referenced this pull request Jun 25, 2019

WIP: fixes lbryio#2510

d7145ed

jessopb force-pushed the redux-comments branch from da5c17d to 24c8c58 Compare June 25, 2019 03:29

kauffj requested changes Jun 26, 2019

View reviewed changes

lbry-bot assigned NetOpWibby and unassigned kauffj Jun 26, 2019

jessopb and others added 7 commits June 26, 2019 20:05

Draft of CommentCreate and CommentList

86e1743

more redux comments

6cbade6

Optimizing

90d2f36

Finalized styling

69cb2b8

comment intro

9add21d

undoes unnec. redux wiring

31178c7

flat comments

99c85a2

jessopb and others added 3 commits June 26, 2019 20:06

comments cleanup

97ba09b

fix yarn.lock

42a632e

cleanup

a4d103c

neb-b force-pushed the redux-comments branch from 97a7625 to 95f759c Compare June 27, 2019 00:07

update lbry-redux

b8949fb

neb-b force-pushed the redux-comments branch from 95f759c to b8949fb Compare June 27, 2019 00:12

neb-b merged commit d7b4e91 into master Jun 27, 2019

lyoshenka deleted the redux-comments branch August 5, 2019 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comment Styling #2510

Comment Styling #2510

NetOpWibby commented May 24, 2019

neb-b left a comment

neb-b Jun 20, 2019

neb-b Jun 20, 2019

jessopb Jun 24, 2019

neb-b Jun 20, 2019

neb-b Jun 20, 2019

neb-b Jun 20, 2019

neb-b Jun 20, 2019

neb-b Jun 20, 2019

osilkin98 Jun 24, 2019

osilkin98 Jun 24, 2019

kauffj left a comment

kauffj Jun 26, 2019

kauffj Jun 26, 2019

jessopb Jun 26, 2019

kauffj Jun 26, 2019

kauffj Jun 26, 2019

kauffj Jun 26, 2019

osilkin98 Jun 26, 2019

kauffj Jun 26, 2019

kauffj Jun 26, 2019

kauffj Jun 26, 2019

kauffj Jun 26, 2019

@@ @@ -28,6 +28,7 @@ export const selectUpdateUrl = createSelector( @@
                 }
               );
+              // HERE

@@ @@ -373,7 +373,7 @@ export function doChangeVolume(volume) { @@
                   });
                 };
               }
+              // HERE

Comment Styling #2510

Comment Styling #2510

Conversation

NetOpWibby commented May 24, 2019

PR Checklist

PR Type

Fixes

What is the current behavior?

What is the new behavior?

neb-b 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

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

kauffj 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

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