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.
RFC for Pull Request Review #1706
RFC for Pull Request Review #1706
Changes from 13 commits
43b33e36af296048a31dc438a9e6bcc742de5410a33b7ac4a85ba758194d36281a53e012532be3279a26d902dcf64f24330cb878ee50fa72884c39dc37fe1fc45f9de792d9d05465f71ea33834d4b153b6bccb4b759884aaf4c79463137442ab74b5a7b7baecd41db429c6a6e58863e1ea3973b2750281c44566817ec937e6c34abbad57705ad24e6File 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.
Is the main purpose to have a quick way to navigate to the "Reviews" tab? Or to keep an eye on new reviews? For the later, some sort of "new comments since I last visited", might be handy, but comes with its own challenges. It also looks a bit crowded with all the icons and numbers.
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.
Yes 馃槅 It's also to start using the PR result tiles to show a bit more information without having to click through, and to give a little symmetry to the information you see for current and non-current PRs.
I'm not 100% sold on it myself though mostly because:
So if it's just too messy we can yank it.
Sidenote:
I'm deliberately punting anything related to notifications about new information arriving from this RFC. It's still important and a place we can add a ton of value, but I believe it's something we should address in a consistent way across the package as a whole (controls that let you opt-in to Atom notifications? Blue-bars to signal "there's something new here"?). It's also something that's going to take a lot of us getting it wrong before we figure out how to get it right.
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.
Maybe we could show the "reviews" progress bar?
And maaaaybeeee also a "tasks" bar.
Then you can glance over the PRs and get a sense of their progress. Like " 馃槺 Oh, this looks almost ready, I better add my 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.
Okay, I definitely like yours better than my mockup, but I'm still not convinced we shouldn't just leave non-current PR tiles alone. I'm kind of leaning toward not touching it I guess?
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.
Something to consider: prompting the user to see if they want to check out the corresponding branch associated with the PR. Prompts can be annoying and sometimes feel intrusive. That said, if the user wants to make use of our fancy inline comments, they'll want to make sure that they are on the correct branch.
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.
Maybe when you're on a non-current PR, we could show a banner somewhere in the
IssueishPaneItemtab that tells you to check out the PR to see inline review comments? Less intrusive than a modal but still a nudge that you're missing the full experience.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.
So this seems to be equivalent to what dotcom shows under the "Conversation" tab for a PR. In our IssuishPaneItem we have an "Overview" tab which is similar to the "Conversation" tab on dotcom (but currently not interactive, namely you can't add comments yet). In our "Overview" tab we currently have timeline events like commits, references, and PR comments (standalone comments that are not associated with a particular review). The contents of the PR comments could be relevant to review comments.
So I'm wondering what makes sense in terms of how to divvy up information between our "Overview" tab and our "Reviews" tab. Does it make sense to combine them into one tab like on dotcom? Users would find this familiar and we could keep all of the comments (PR and review comments) in a single tab. On the other hand, it might be nice to have a dedicated "Reviews" section, because there's already a lot of other stuff going on in the "Overview" tab.
It would be good to get clear on the boundary and relationship between these two tabs if we want to have them both. Without having a strong narrative here, I would opt for combining them like dotcom has under the "Conversation" tab.
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.
That's an interesting question. Sometimes I wonder how it would feel if PR and Review comments are strictly separated?
The only problem, I think you mentioned earlier, there is no place to make review comments without choosing a line. Review summaries could be used, but don't have replies and I think get overridden, when making a new review.
Somehow I'm still intrigued to try the separation, maybe because reading reviews in the conversion AND changes feels like double work?
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.
Worth nothing -- my initial assumption was that the "diff" button would navigate to the "Changes" tab, not the "Reviews" tab. That said, I don't necessarily think that it's better to navigate to the "Change" tab...
Some good question to determine what makes sense might be --
"What information does the user care most about when clicking this button?"
"What action is the user interested in making next?"
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 this is a good point. It would be handy to offer a quick and easy way to toggle comments visible/hidden. Like a command/keyboard shortcut/easily accessible button.
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.
I need some clarity on when these are being viewed. I would argue that you'd only want to see comments when you're in a review state, not in an editing state, making this a non-issue.
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 do have some state we can use if you're writing a pending review, but I don't think we have specific state right now that indicates whether or not you're actively reading reviews. Is there a natural place we can introduce that?
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.
In a world of multi-file editable diffs, I wonder if we could combine some of these in a natural and fluid way...
Something in my gut tells me that the "Changes" tab and the "Reviews" tab is too much / redundant / confusing... And they seem like the belong in the dotcom paradigm, whereas in our editor we want to create an entirely new paradigm around the fact that we are in the context of the user's working directory. Hence the power and beauty of editable diffs...
Here's what I'm picturing.... the picture is incomplete, just ideating here.
We could have a single view that shows all changes as a conversation among reviewers, and this view can be "filtered" to just show the changes for a single reviewer. This satisfies much of bullet points 1 and 2 above. I'm imagining a summary header that is always visible (floats there while the user scrolls much like the file-navigator on dotcom) that has quick summaries of each review and allows you to filter based on that review with a quick click. This header would include the progress information.
This view could be a multi-file editable diff that allows users to quickly apply changes to their code without having to click a "code" (<>) button and hop to a different view. This satisfies much of bullet point 3. Users may still choose to jump to a code file to get more context (say they want to view a function definition that is located elsewhere in the file and can't be seen by expanding context lines), but in the majority of cases I think users would simply want to edit the diff and incorporate changes from the comments.
With a single multi-file editable diff view we could potentially cover the bulk of the three scenarios described in a single view. Minimizing the back-and-forth and (hopefully) reducing the complexity of our UX (but maybe increasing it in other ways 馃 ).
I think the main point I want to make is that it feels a bit funky to me to have dotcom-style diff views in our IssueishPaneItem. We are in an editor. Users will most likely want to edit their source. Maybe we consider a editor-first solution?
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.
Oh, wow. Holy shit! This is a super cool idea.
Were you picturing this as the way we do the "Changes" tab (called that or something else)? Or is this something we add inline to the "Overview" tab somehow so it's colocated with the rest of the conversation?
Any thoughts on how it's ordered - do we render the files in diff order and attach comments according to placement? Or do we try to preserve the chronology of reviews somehow?
What do we do when you're looking at this view on a non-current pull request? If we had first-class worktree support, I could see us silently creating an isolated worktree for the PR and making your changes there.
I can also see it being confusing that an editable PR diff has different behavior than an editable FilePatchItem diff:
I'd been thinking about editable diffs in a FilePatchItem as a rough equivalent editing the diffs in
git add -porgit add -i, so you could do things like "delete a.only()in a Mocha test before it gets committed, leaving your working copy file alone." But we could also change how we do that to be more consistent with this 馃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.
馃憤 I like that 馃槃
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!! Some friends of mine wrote a Chrome extension to change the "request changes" icon on dotcom to 馃檹. Changing the icon can help a lot here to improve the tone.
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.
Okay, I kind of want to use the 馃檹 emoji now...
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.
I think @simurai's icon choices above are great too! .As long as we're not using the red icon dotcom uses, which feels pretty heavy handed to me, I'm open to a wide range of options.
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.
I love this question. And I love these suggestions. This sort of discussion and exploration makes me so happy :)
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.
Maybe even just emoji reactions to lines of code