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
Add support for multi-line comments. #1566
Conversation
This adds support for the "comfort-fade" preview, which enables multi-line comments (incl. suggested edits). I have been carrying a subset of this as a patch downstream for a while, which just hard wired things on, but I tried to make this "smart" for back-compat as part of upstreaming.
Codecov Report
@@ Coverage Diff @@
## master #1566 +/- ##
==========================================
- Coverage 67.90% 67.90% -0.01%
==========================================
Files 95 95
Lines 8722 8750 +28
==========================================
+ Hits 5923 5942 +19
- Misses 1892 1898 +6
- Partials 907 910 +3
Continue to review full report at Codecov.
|
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.
Thank you, @mattmoor!
Do you want to add any comments to the CreateReview
method documentation describing how to use this multi-line comments feature? The announcement on https://developer.github.com/changes/2019-10-03-multi-line-comments/ is rather light on documentation.
Your call. Otherwise, one minor tweak please, and then this PR LGTM and we will be ready for a second LGTM and then a merge.
Yeah, ack on the docs. I spent a few days beating my head against their API (and a few twitter DM convos with their PMs) to figure out the various ways I was holding things wrong. I added a detailed comment, which reflects my observations (Good old, Dr. Empirical). Hopefully it will keep others from falling into the same traps I did. |
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.
Thank you, @mattmoor!
LGTM.
Awaiting second LGTM before merging.
@gmlewis Anything I can do to help move this along? 😇 |
You can always ping @wesleimp - he is one of the most responsive volunteer reviewers we have. 😄 |
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 👌
See what I mean?!? 🤣 |
This adds support for the "comfort-fade" preview, which enables multi-line comments (incl. suggested edits).
I have been carrying a subset of this as a patch downstream for a while, which just hard wired things on, but I tried to make this "smart" for back-compat as part of upstreaming.