Added github-handle for Sanya Nijhawan in home-unite-us.md#7261
Added github-handle for Sanya Nijhawan in home-unite-us.md#7261t-will-gillis merged 1 commit intohackforla:gh-pagesfrom
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. |
|
Availability: Sun/Mon all day, Tues-Fri evenings |
daras-cu
left a comment
There was a problem hiding this comment.
Hi @joooseph2, thanks for working on this issue!
What looks good:
- You added
github-handleas described in the issue. - When I test the website with your changes there are no visual changes and everything displays correctly.
- You are merging from your branch to the correct gh-pages branch.
- You linked the original issue number in the PR.
A couple things to address:
- The name of the branch that you created includes the issue number, but it should be a little more descriptive so someone reading it has a general idea of what issue it addresses. I'm not sure if this can be changed at this point without redoing the PR, so may be something to keep in mind for the next issue you work on.
- Under "Why did you make the changes", your answer should show you understand the purpose of the changes for the website. If you take a look back at the original issue #7181, the
github-handlevariable is going to replace two other variables to reduce redundancy, so your answer here could reference that. - Under the screenshot section, instead of saying you added a new variable you can just put "No visual changes" so it's more clear for reviewers that the website visuals should not be affected.
- Don't forget to check off the action items in the original issue as you complete them.
A good idea may be to review successfully merged PRs for similar issues to see what they should look like, for example #7325.
Once you've made adjustments let me know in a comment or re-request a review and I will take another look!
|
@daras-cu Thanks for the feedback! I've done each item. For the next pr I'll make sure to do a better branch name. |
daras-cu
left a comment
There was a problem hiding this comment.
Looks great, thanks for making the changes!
pluto-bell
left a comment
There was a problem hiding this comment.
Great work on this issue and also in handling the changes requested by daras-cu. Everything has been cleaned up and it looks like you now have a better idea of what to do moving forward.
(:
|
@t-will-gillis could I get a merge for this to work on my second skill issue? Also what is the standard protocol for getting issues reviewed? Do I just have to wait for someone to pick it up or should I just directly ask. I want to keep working on the skills issues but there's a decent delay between approvals and merges. |
|
Hi @joooseph2 - done! As to the protocol for getting things reviewed, yes, unfortunately you have to wait. You can try asking, which might work if you ask someone who also has an open PR pending review. (You did not ask this, but you do not want to assign reviewers even though GitHub suggests people. Often this backfires b/c the suggestions are for people who are no longer active and you will be waiting a very long time.) We try to keep the process moving through having the On call reviewers and asking for volunteers during the weekly meetings. Finally, while you are waiting for a review it really helps moving things along if you do several PR reviews yourself. |
|
@joooseph2 If you are waiting a long time for a reviewer to complete a review or if all reviews are completed and you just need it merged, definitely feel free to ask like you did. |
Fixes #7181
What changes did you make?
Changed variable
name:in /website/_projects/home-unite-us.md to include variablegithub-handle:name: Sanya Nijhawanname: Sanya Nijhawan github-handle: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)