This repository has been archived by the owner on Dec 15, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature Request: Review author flow #2095
Feature Request: Review author flow #2095
Changes from all commits
781f088
a9ee940
942238e
da7023e
6682d2e
52e9782
83f7ced
e309b2f
b74359b
a3e0e69
99cfc64
40109d2
98d67c6
4f10bd7
13ecc2f
999af2f
87b7559
f790be8
b6c4789
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hmm... since the pending review will be in the "All Reviews" tab, is it necessary to have a separate "Pending Review" tab?
Quite possibly. I haven't thought too deeply about it. Just wanted to throw out the question.
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.
My major concerns with keeping both published comments and pending comments in the same space are that 1) it would get too crowded, and 2) there would be no obvious hierarchy in the information presented.
I would argue that when a user is in the middle of reviewing a PR (i.e. have a review pending), the most relevant information to them would be their pending comments, as well as the summary textbox that they need to fill out before publishing a review. I couldn't figure out a way to fit all this along with other published comments, while preserving the hierarchy of importance.
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.
👍
We can fiddle with the "in focus or not empty" behavior once we have an implementation to play with. I'm remembering that we had to scrap the "comment editor that expands when focused" because it made the surrounding UI elements jolt around.
It also feels a little awkward to have two rows of buttons right next to one another. Not sure what else we could do there, though 🤔
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.
Oooh interesting. I kinda like this placement, and I like the arrow that indicates it'll take you to somewhere else if you click it.
Minor UI question...
I think our current implementation has the badge on the top of the comment next to the author name. Should we consider keeping it up there? Or do we want to make it more pronounced like it is here?
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.
Hrmm yea honestly I am 50/50 on this one. I am open to fiddle with it once we are at implementation stage, and then decide which treatment has better UX.
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.
Yeah - the existing "pending" badge is at the top. But it also has no click action. I think being actionable would justify moving it below though. 🤔
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.
Does this imply we won't have a default? Looks like dotcom has "Comment" as default (at least for me)
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.
Hrmm.. I can see the value in both having "Comment" as a preseleted default, and making users explicitly choose the type of review they're making. I think I am indifferent? Do you have a preference for one?
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.
Preselecting it would reduce the interactions required to submit a comment review, but increase the number of users who submit one by accident or without thinking. I'm kind of indifferent too, but I think slightly on the side of not having a default and making the user make a conscious choice... ?