-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
comments: Add proposal author updates. #2549
Conversation
<Card className={classNames("container", styles.commentsHeaderWrapper)}> | ||
<LoggedInContent | ||
fallback={ | ||
<WhatAreYourThoughts | ||
onLoginClick={handleOpenLoginModal} | ||
onSignupClick={onRedirectToSignup} | ||
/> | ||
}> | ||
<Or> | ||
{readOnly && ( | ||
<Message kind="blocked" title="Comments are not allowed"> | ||
{readOnlyReason} | ||
</Message> | ||
)} | ||
{!isPaid && paywallEnabled && currentUser && ( | ||
<Message kind="error"> | ||
<P> | ||
You won't be able to submit comments or proposals before | ||
paying the paywall, please visit your{" "} | ||
<Link to={`/user/${userid}?tab=credits`}>account</Link> page | ||
to correct this problem. | ||
</P> | ||
</Message> | ||
)} | ||
{!readOnly && !!identityError && <IdentityMessageError />} | ||
{areAuthorUpdatesAllowed && !isCurrentUserProposalAuthor && ( | ||
<Message> | ||
Replies & upvotes/downvotes are allowed only on the latest | ||
author update thread. | ||
</Message> | ||
)} | ||
</Or> | ||
{!isSingleThread && | ||
(!readOnly || | ||
(areAuthorUpdatesAllowed && isCurrentUserProposalAuthor)) && | ||
proposalToken && ( | ||
<CommentForm | ||
persistKey={`commenting-on-${tokenFromUrl}`} | ||
onSubmit={handleSubmitComment} | ||
disableSubmit={!!identityError || paywallMissing} | ||
isAuthorUpdate={ | ||
areAuthorUpdatesAllowed && isCurrentUserProposalAuthor | ||
} | ||
/> | ||
)} | ||
</LoggedInContent> | ||
{commentsError && ( | ||
<Message kind="error" className="margin-top-m"> | ||
{commentsError.toString()} | ||
</Message> | ||
)} | ||
</Card> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be in container/Comments/Comments.jsx
but as it could be displayed now multiple times for each
author update section this logic moved to container/Proposal/Detail/Details.jsx
as it should be displayed
once in the proposal details page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just found a bug when viewing single thread using comment specific link while having multiple author update threads
Fixed. Another minor bug I just found, if proposal author posts a new author update it goes to the bottom while |
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Comment plugin errors need human readable messages. Note, this error was caused by me not having my comments
allowextradata
plugin setting set to true, so it was not caused by something in this PR.
- The update title should be the title for the comments section, not in the comment itself. Example: the
Comments
text fromComments (1)
in the image below should be the update title.
- If I was a proposal author that was using this feature for the first time, I would not really know how to use it. The rules of proposal updates are not clear from the existing UX. Here are some things I think a first time user would struggle with.
-
How am I suppose to know that the comment markdown editor is suppose to be used for proposal updates? It looks like I'm leaving a normal comment. I probably wouldn't know that this is for author updates only and would likely assume that this is availble for every user.
-
How would I know that creating a new update locks the old thread? I would probably assume that I can use this like normal comments, which could lead to me creating multiple updates instead of replying to comments in the original thread.
We should talk through the UX for this in the pi-dev channel.
Everything else appears to be working nicely. We just need to work through some of these UX details.
Addressed. |
author update comments section.
Addressed. |
info for the author about the feature.
tmp.mp4 |
Same on master, opening a separate PR with a fix. |
Fixed on #2582. |
e2e tests added ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @amass01 thanks for the PR. Code is looking clean and good. Thanks for addressing my comments. Left minor inline comment regarding mocking test data.
Also, I think you could add more e2e tests to cover more actions over the author updates, for example:
- If I submit an author update for the first time, will the author updates be listed correctly?
- Submitting a new update locks the previous thread?
- Can I submit author updates on not approved proposals?
- Can I upvote/downvote an author update?
- Should I be able to reply author updates?
You can open an issue to track these e2e tests if you think it's more appropriate, but I think they would be a good addition :D
I already cover the following two:
But I also cover more:
I think this is more than enough for the scope of this PR and of course we can add more |
Is this ready for a final review? |
@lukebp yep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
Nice work @amass01. This is looking real nice.
Needs final code approval from @tiagoalvesdulce. @victorgcramos can you also give this a final review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Good job on e2e tests cleanup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is looking pretty good. Great job! I left a few comments.
} | ||
|
||
// Convert comment array to a map | ||
const commentsMap = comments.reduce((map, comment) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
Spec
Proposal authors need to be able to give periodic updates on the status
of their proposal. These updates will be given as comments. The Politeia
users will have the ability to interact with the author and ask further
questions.
Before this commit, once a proposal vote has finished the proposal is
locked and no additional comments can be made. This commit changes
that to the following:
Once a proposal vote has finished, all existing comment threads are
locked.
When a proposal author wants to give an update they will start a new
comment thread. The author is the only user that will have the
ability to start a new comment thread once the voting period has
finished. Each update will have an author provided title.
Anyone can reply to any comments in the thread and can cast
upvotes/downvotes for any comments in the thread.
The comment thread will remain open until either the author starts a
new update thread or an admin marks the proposal as
closed/completed.
implementation
Each update is shown as its own comment section. The updates will be
displayed from newest to oldest.
Each author update has a title - comment form updated & title
displayed if the comment in an author update.
This diff refactors/normalizes the comments redux state, by
storing the comments as a map vs as an array before this.
This improves the performance and avoids extra expensive
re-renders on the containers level where they need to process the
comments array to split the comments to different sections in case
the proposal has author updates which are separated threads from
the main comments thread.
Closes #2547.
Closes #2570.
Depends on decred/politeia#1491.