Skip to content
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

Make review comments editable #2124

Merged
merged 93 commits into from May 15, 2019
Merged

Make review comments editable #2124

merged 93 commits into from May 15, 2019

Conversation

@kuychaco
Copy link
Contributor

@kuychaco kuychaco commented May 4, 2019

Starting work to address #2112. Will add writeup shortly!

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

  • Editable review comments
  • Editable review summaries
  • Focus on textbox when editing is enabled
  • Keyboard shortcut (cmd+enter to submit and esc to cancel)
  • Use the markdown parsing bits from #2102 (pending its merge)
  • Use unified relay query error reporting from #2128 (pending its merge)
  • Include "edited" state in the response from the edit mutations and optimistic results
  • Only show the edit option if user has permission
  • Test coverage

Screenshot/Gif

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Metrics

Tests

Documentation

Release Notes

User Experience Research (Optional)

@vanessayuenn vanessayuenn added this to In progress in PR review author feature May 7, 2019
smashwilson added 5 commits May 10, 2019
This will let cmd/ctrl-enter fire the github:submit-comment event for 
review summary editors and existing comment editors, too.
Without this, the "edited" flair doesn't appear on a review comment or 
summary you edit until you perform a full refetch.
We added a few fields to PullRequestReview and PullRequestReviewComment, 
so we need to specify some defaults.
@codecov
Copy link

@codecov codecov bot commented May 10, 2019

Codecov Report

Merging #2124 into master will increase coverage by 0.04%.
The diff coverage is 98.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2124      +/-   ##
==========================================
+ Coverage   92.68%   92.72%   +0.04%     
==========================================
  Files         213      215       +2     
  Lines       12188    12269      +81     
  Branches     1788     1796       +8     
==========================================
+ Hits        11296    11376      +80     
- Misses        892      893       +1
Impacted Files Coverage Δ
lib/containers/reviews-container.js 100% <ø> (ø) ⬆️
...ainers/accumulators/review-comments-accumulator.js 100% <ø> (ø) ⬆️
lib/items/reviews-item.js 100% <ø> (ø) ⬆️
...iners/accumulators/review-summaries-accumulator.js 100% <ø> (ø) ⬆️
lib/controllers/root-controller.js 82.87% <ø> (ø) ⬆️
lib/views/actionable-review-view.js 100% <100%> (ø)
lib/views/review-comment-view.js 100% <100%> (ø)
lib/controllers/reviews-controller.js 100% <100%> (ø) ⬆️
lib/views/reviews-view.js 87.11% <80%> (-0.69%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14079a6...8e4b08f. Read the comment docs.

Loading

smashwilson and others added 13 commits May 10, 2019
I've removed the callback parameter - using Promise fulfillment will be 
cleaner - and made the update methods re-throw caught mutation errors. 
I've also updated the callsite in ActionableReviewView to use await and 
try/catch to manage editing state. Now if the update mutation fails, 
users won't lose their edits.
This lets us inject mocks to test menu actions without having to do any 
crazy Electron shenanigans.
I've got everything but the cancel button tests.
@vanessayuenn vanessayuenn marked this pull request as ready for review May 13, 2019
kuychaco and others added 2 commits May 14, 2019
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Copy link
Contributor Author

@kuychaco kuychaco left a comment

Looks great to me! Left a couple of comments, nothing blocking.

If I weren't the PR author I'd submit this with an "Approve"

Loading

lib/views/review-comment-view.js Outdated Show resolved Hide resolved
Loading
await this.props.contentUpdater(this.props.originalContent.id, text);
this.setState({editing: false});
} catch (e) {
this.buffer.setText(text);
Copy link
Contributor Author

@kuychaco kuychaco May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'd get to this catch block when the update mutation fails, and if I'm understanding Relay mutations correctly then the optimistic update will automatically get rolled back, and the original text will be restored to the buffer. So I think it may not be necessary to manually restore the text here... though I guess it couldn't hurt to do so and be 100%.

Loading

@vanessayuenn vanessayuenn merged commit f650f69 into master May 15, 2019
17 checks passed
Loading
Release : 9 May 2019 - 5 June 2019 : v0.30.0 automation moved this from In progress to Merged May 15, 2019
@vanessayuenn vanessayuenn deleted the ku-edit-comments branch May 15, 2019
@smashwilson smashwilson moved this from In progress to Done in PR review author feature May 15, 2019
@smashwilson smashwilson mentioned this pull request Jul 16, 2019
@smashwilson smashwilson mentioned this pull request Jul 18, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants