-
Notifications
You must be signed in to change notification settings - Fork 405
Add author association to review summaries and thread comments #2085
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2085 +/- ##
==========================================
+ Coverage 92.55% 92.55% +<.01%
==========================================
Files 207 207
Lines 12016 12021 +5
Branches 1745 1746 +1
==========================================
+ Hits 11121 11126 +5
Misses 895 895
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.
looks good!! Thanks for taking this on.
@@ -113,6 +113,59 @@ describe('ReviewsView', function() { | |||
assert.lengthOf(wrapper.find('details.github-Review'), 2); | |||
}); | |||
|
|||
it('displays an author association badge for review summaries', function() { | |||
const {summaries, commentThreads} = aggregatedReviewsBuilder() | |||
.addReviewSummary(r => r.id(0).authorAssociation('MEMBER')) |
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.
Thanks for being thorough and testing all the associations!
I really want you to not have to type all these things out manually. I wish it was possible to do something like...
const {summaries, commentThreads} = aggregatedReviewsBuilder();
let idx = 0;
Object.entries(authorAssociationText).forEach(([key, value]) => {
addReviewSummary(r => r.id(idx).authorAssociation(value));
idx ++;
});
summaries.build();
comments.build();
I don't think we can actually use the builders that way though.
I mean, you could create a new review and summary for each association and then test those. Since you already put in the effort to manually type all these out, not sure it's worth the time to optimize.
…line Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
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.
Thanks for tackling this!
Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.
Requirements
Description of the Change
Our trusty Community and Safety team pointed out that displaying "first time contributor" badges encourages people to be kind to new folks, etc.
This PR introduces
authorAssociation
information provided by the GitHub GraphQL API to indicate the author's relationship to the repository (member, owner, collaborator, contributor, first-time contributor, first-timer, none). See API docs for more information.Badges for author association are displayed on review summary comments as well as review thread comments.
See #2056 for reference.
Screenshot/Gif
Summaries:
Review threads:
Alternate Designs
None were considered.
Benefits
Folks have more context about the author of the comments and can therefore adjust their responses accordingly. For example, responses to comments made by "First-time contributors" warrant extra thought and attention, to ensure that we are welcoming new contributors and effectively growing our communities -- https://github.blog/2017-07-25-making-it-easier-to-grow-communities-on-github/
Possible Drawbacks
There is limited space in comment headers. An additional badge adds some visual noise, and if the panel width isn't large enough, the badge appears on a separate line:
Applicable Issues
#2056
Metrics
N/A
Tests
Added unit tests to ensure that text shows up correctly for each author association value (member, owner, collaborator, contributor, first-time contributor, first-timer, none). Did this for both review summary comments and thread comments.
Documentation
N/A
Release Notes
User Experience Research (Optional)
N/A
TODO: make summary badges drop to new line (as is the case for thread comments)