-
-
Notifications
You must be signed in to change notification settings - Fork 776
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
Added Mira Kinebuchi's github handle variable in home-unite-us.md #7315
Added Mira Kinebuchi's github handle variable in home-unite-us.md #7315
Conversation
added github-handle for Mira Kinebuchi
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.
|
Review ETA: EOD Tue Aug 20
|
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.
Hi @mSharifHub,
What went well:
- Into branch is set up correctly.
- PR is linked to the correct issue number.
- Branch is named correctly
- Only relevant lines of code were changed.
- You answered the "What.." and "Why.." sections well!
Changes needed:
- PR title
- The current PR title does not accurately represent the changes. While the file mentioned was updated, I think the title could be more precise.
- PR Comment
- The first line in the comment is redundant, as the "What.." section has that information.
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.
Hey @mSharifHub!
Great job correctly linking the issue and explaining your what and why.
Like Balpreet mentioned, the first line in your PR comment can be removed and the PR name can be a bit more descriptive of what was updated. For instance "Added Mira Kinebuchi's github handle variable in home-unite-us.md".
Everything else looks great!
ETA: EOD |
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.
Hi @mSharifHub,
I'll just touch on things that other reviewers have not mentioned.
- I think your branch name could be slightly more descriptive, you don't have to add that change for this PR but keep it in mind for your next one.
- In your PR, some of the bullet points are not formatted properly. Simply edit the PR to add spaces after every "-".
Just make the simple formatting change and rerequest the review.
I do not know how descriptive more can I be ? Do i need to explain what is github handle for ? I dont get it. It has been over a week i have submitted. Over one line of code with a description from my git submission did no stop anyone from finding the issuse since I already have it under fixes. I do not see how productive this is but very counter productive |
@mSharifHub, @mmcclanahan gave you can example you could use; you could just copy paste that. Your changes resulted in an update to the file you mentioned but that was not your change nor the goal of the PR. Your change was adding the github-handle key. At minimum, the title should mention that. It seems you're having trouble with this, so I'll give you what my answer would be: Added github-handle for Mira Kinebuchi. I understand it can be frustrating to wait. You're almost there, please update the title and I will approve. |
Review ETA: 8/26/24, 11:59pm PT |
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.
Great job on the branch name—it looks good! Just a quick note: for future reference, it would be helpful to include the specific change you made in the title. This can make it easier for everyone to quickly understand what the branch is about.
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.
It seems good now. Approved!
Hi @mSharifHub Just to make sure everyone is on the same page: there are changes requested above that have not been addressed yet, and the PR can not be merged until those requested changes have been addressed. Here are the two main items that appear to be outstanding:
|
sounds good. will fix now
…On Sun, Sep 1, 2024 at 3:27 PM Will Gillis ***@***.***> wrote:
Hi @mSharifHub <https://github.com/mSharifHub> Just to make sure everyone
is on the same page: there are changes requested above that have not been
addressed yet, and the PR can not be merged until those requested changes
have been addressed.
Here are the two main items that appear to be outstanding:
- @mmcclanahan <https://github.com/mmcclanahan>, @minkang3
<https://github.com/minkang3>, and @8alpreet
<https://github.com/8alpreet> and mentioned editing the title of the
PR and gave examples of what the title could be changed to, for example
@mmcclanahan <https://github.com/mmcclanahan> suggested: "Added Mira
Kinebuchi's github handle variable in home-unite-us.md". Please make the
change to the title. Select "edit" as shown below to make the edits
- @minkang3 <https://github.com/minkang3> requested that you add
spaces between the hyphen and text in the description. Please add the space
between the hyphen and the text in the two locations shown below.
- After you have made the changes above, please e-request reviews from
the reviewers.
Screenshot.2024-09-01.151840.png (view on web)
<https://github.com/user-attachments/assets/ac70baee-c1b8-4bee-a5e0-cb7f1ee4db26>
—
Reply to this email directly, view it on GitHub
<#7315 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BAIFC5ADAZL46HNGHZSF7ADZUOIGPAVCNFSM6AAAAABMYF654SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGUYTSMZTGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I have made changes. I added a tab to each as space |
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.
🙏
…On Sun, Sep 1, 2024 at 7:41 PM Min Kang ***@***.***> wrote:
***@***.**** approved this pull request.
Looks good.
—
Reply to this email directly, view it on GitHub
<#7315 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BAIFC5EUTOXB5QRGZEOP6CDZUPF65AVCNFSM6AAAAABMYF654SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZUGU4TCMZVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
Great Work!
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.
Hi @mSharifHub,
You missed the first of @t-will-gillis's bullet points; the PR title still needs to be changed.
Fixes #7177
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)