add github-handle to member Tan Zhou#6215
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL: |
|
@awlFCCamp The change in code looks good as I pulled it into docker. I see in #6192, spaces instead of tab for .yml Before I approve review, I see inconsistent comment style under visuals with 404 links. If no visual change, please comment underneath with no visual change under screenshots heading :). Thanks! |
|
@njackman-2344 Thank you for reviewing my PR. I have updated my PR, removed<details> and <summary>, and replaced them with an explanation of no visual changes to the website.
On Friday, February 2, 2024 at 08:28:53 PM CST, njackman-2344 ***@***.***> wrote:
@awlFCCamp The change in code looks good as I pulled it into docker.
I see in #6192, spaces instead of tab for .yml
Before I approve review, I see inconsistent comment style under visuals with 404 links. If no visual change, please comment underneath with no visual change under screenshots heading :). Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@elliot-d-kim Thank you for reviewing my PR. I added information about the modified file and the reason for the change. There are no visual changes to the website, so I followed the instructions deleted <details> and <summary>, and added the comment of no visual changes to the website.
On Friday, February 2, 2024 at 08:59:18 PM CST, elliot-d-kim ***@***.***> wrote:
Hey @awlFCCamp, this is looking pretty good!
- Branches and naming was done properly,
- issue is linked, and
- code looks clean and accurate!
Just a couple of suggestions:
- A little more detail would make your summary great (e.g., the file you made this change in). Also, to answer why the changes were made, consider what problem your changes are solving. I would look at the Overview in the issue to get a better understanding. Refer to this section for an example of a good summary: 3.1.b.iii Complete pull request (3): Explain the changes you made, then explain why these changes were needed
- I'm unable to see the before and after screenshots included in this post. Hopefully this section will help with that: 3.1.b.iv Complete pull request (4): Include images (if available)
Let me know if you need clarification!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I accidentally posted my review as a comment so I deleted it. I see that you've responded to it (I guess it was still available in your email inbox) and I'll formally review the PR properly by the end of the day! Thanks for understanding. |
elliot-d-kim
left a comment
There was a problem hiding this comment.
Hey @awlFCCamp, this is looking pretty good!
- Branches and naming was done properly,
- issue is linked, and
- code looks clean and accurate!
The summary is in great shape after the adjustments you made: good detail and reasoning. Thanks for also cleaning up the screenshots section!
By the way, no need to keep any empty bullet points. Deleting them would make your PR cleaner -- just something to keep in mind moving forward.
Thanks again and nice work, I'm happy to approve this :)
njackman-2344
left a comment
There was a problem hiding this comment.
Branches and naming looks good and accurate
issue is linked
code great
Thank you for comment consistency with no screenshot changes at bottom
PR Approved
Fixes #6192
What changes did you make?
Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
No visual changes to the website.