-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
More etiquette for responding to code reviews #2169
Conversation
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.
Temporarily "requesting changes" here so that I can get a screen shot of the UI after doing so...
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. Suggested an addition below that might be helpful, let me know if you'd like me to look at an update, or send this in a follow-up.
docs/project/code_review.md
Outdated
for re-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.
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.
Added, with some text.
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.
Just tweaking wording as the tool-tip (and screen reader) for the button seems to be "re-request review". Still LGTM, feel free to submit w/ these suggestions and any wording/grammar fixes needed.
Co-authored-by: Chandler Carruth <[email protected]>
Includes the advice from #2022 (comment)