This repository was archived by the owner on Jun 21, 2023. It is now read-only.
This repository was archived by the owner on Jun 21, 2023. It is now read-only.
Known issues in PR reviews beta #1570
Description
Known issues:
- Some comments won't open inline Display PR review comments inline. #1549 (comment)
- Appears to be the same as Pull Request inline comment appears on GitHub.com but not in extension #1469 - needs an update to libgit2sharp
- Each time I click a comment the entire diff view reloads Display PR review comments inline. #1549 (review)
- No client-side validation for body on "Request Changes" Start PR Reviews from Inline Comments #1562 (comment)
- Cancel button not working Start PR Reviews from Inline Comments #1562 (comment)
- Quotes in review body not escaped: Start PR Reviews from Inline Comments #1562 (comment)
- Octokit.GraphQL fix is here Quotes and backslash are not escaped. octokit/octokit.graphql.net#72
- Fixed by Fix Octokit.GraphQL escaping issue. #1581
- Feature does not work with GitHub Enterprise (trying to open PR's shows: "Object reference not set to an instance of an object.")
- Navigate from diff view to editor feature is missing
- Fixed by Fix navigate to editor. #1580
- Approve/Comment/Request Changes links need a disabled visual state
- Fixed in Validate PR review state before allowing submission. #1582 (see HACK section ;) )
Ongoing discussions:
- A user can comment on their own PR but dotcom will never show a user as a reviewer of their own PR under Reviewers explicitly. So I'm just wondering if the goal for the reviewers section is to behave like dotcom behaves or not. Add "Reviewers" section to PR details. #1523 (comment)
- Because we don't yet have a conversation view, there would be no way to see these reviews if we didn't include them here, so we think we need to keep this behavior, at least for now
- I wonder if we should display the total number of comments (for what it's worth, once someone has reviewed or commented on a PR, then they will always appear in the list.) Add "Reviewers" section to PR details. #1523 (comment)
- Dogfooding this, being able to see the x file comments is kind of useful. I wonder if that could be incorporated in a more condensed way?
Add "Reviewers" section to PR details. #1523 (comment) - Definitely! 😄I wanted to remove that for now because it wasn't clear that the number of comments was for last review and not total comments. However, I think what would be really useful is tracking how many comments still needs to be addressed (and maybe across a number of files.) We should consider researching more into this. Add "Reviewers" section to PR details. #1523 (comment)
- Possibly, though other information like submitted date would also be useful. As mentioned comment count is unclear because it only refers the the latest review, when user may be expecting the total number of comments. Needs investigation, but we're not going to block on this.
- Dogfooding this, being able to see the x file comments is kind of useful. I wonder if that could be incorporated in a more condensed way?
- Ordering the comments starting with most recent might make this a more manageable (given that you can't dismiss comments, if I've understood correctly). Add "Reviewers" section to PR details. #1523 (comment)
- I think I'm fine with the ordering as-is. If we want to order by most recent here, we should surface more information so that this is clear. That said, there's an opportunity for us to understand how developers determine which set of reviews to address first (e.g., based on review status, number of file comments, date the review was left.) Add "Reviewers" section to PR details. #1523 (comment)
- Again, something we can explore going forward.
- With this format the ✔️ or 💬 can end up a long way from the username/avatar. Could this be flowed with as many as fit? Add "Reviewers" section to PR details. #1523 (comment)
- Yes, this can look a bit strange, but not sure of decent alternatives at the moment. Needs some @donokuda experimentation.
- What do you think of moving the Reviewers above the Description? To me it doesn't feel right having is sandwiched between the Description and Changes tree. Add "Reviewers" section to PR details. #1523 (comment)
- Consensus was that putting reviewers above description felt a bit strange, that one expects Description to be first. A possible solution to this might be to remember the expanded state of the sections so that once read the description can be collapsed and the reviewers will be immediately visible?
- Could we make it so you can click on either the avatar or the ✔️ / 💬? Add "Reviewers" section to PR details. #1523 (comment)
- I don't see any differentiation between outdated and new reviews Display PR review comments inline. #1549 (review)
- @donokuda wants to explore options for this, though will probably be for a later release.
- When I click on the comment, if it is in a thread of comments for that line, there is no indication of this. Maybe we don't want one, but it was just interesting that I had to search through to find it. That being said, it might make sense for that functionality.
Display PR review comments inline. #1549 (review)- We could pulse the clicked comment when shown. This might be difficult at the moment because the diff view is janky, can maybe investigate when we try to fix that. cc: @donokuda
- For other errors don't we usually have a banner at the top? Start PR Reviews from Inline Comments #1562 (comment)
- We should investigate this in general
- Should pending reviewers be shown like they are in dotcom? Add "Reviewers" section to PR details. #1523 (comment)
- Meant "requested reviewers" - this should be added, but it's a separate feature.