-
-
Notifications
You must be signed in to change notification settings - Fork 778
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 Melissa Perry to the Current Project Team section #7723
added Melissa Perry to the Current Project Team section #7723
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.
|
Review ETA: 10 AM 11/17/24 |
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.
@belunatic Great job on your Pull Request! Thank you for your contribution keep it up !
Review ETA: 11/18/24 |
Hi @maadeshsivakumar You have selected to assign yourself to this PR as shown here: Reviewers do not assign themselves to PRs that they are reviewing. Please go to "Assignees", select the gear, and then un-assign yourself. Thank you! |
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 working on this issue, @belunatic !
- The branch name includes the issue number and is appropriately named.
- The issue is linked correctly.
- Before and after screenshots are provided to show the changes.
- The code changes are correct.
- CodeQL Alerts was checked.
Suggestions:
-
Consider revising the title to make it more specific about the files you changed.
-
In the What changes did you make? section, please specify the file(s) you edited. This would be very helpful.
Example: File changed: projects/tdm-calculator.md -
Check the box on the linked issue (original issue) that says:
"Verify the changes by viewing the following in your local environment and include before and after screenshots with your pull request."
Thank you for your contribution! Keep up the great work!
Review ETA: 6 PM 11/17/24 |
Review ETA: 16 PM 11/17/24 |
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 @belunatic !
Everything is done correctly; the branch name is correct, the issue is linked, and changes are reflected in the new branch and screenshots.
Notes:
-
The title is a little unclear. "I added Melissa Perry to the Current Project Team section" doesn't specify which one, since there are MANY different teams across Hack for LA. It would be good to specify that it's for the LA TDM Calculator team. A link to the section would be nice as well. (The original issue has a link in it's subnotes: https://www.hackforla.org/projects/tdm-calculator )
-
Specifying which file you changed would be nice as well, so we don't have to search for it.
Otherwise, great job!
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 @belunatic everything looks great. Code changes are correct and branches are correct. For next time, please be a bit more descriptive in the Why and What sections as these are very important as issues progress in difficulty and for the rest of the devs to further understand your code.
For example, you could include the path of the file you are editing in the What section.
For completeness also please check all the boxes in the issue #7647 and/or ask if you need help resolving some of them. I understand some may remain unchecked but I see one like "Find the leadership variable and add the following profile." that could be checked.
Besides that, the changes look awesome in my local version of the website. Thanks for working on this!
Hi @belunatic Looks good! Before your next PR, please make sure to read through the comments above - there are many good observations here to keep in mind. Thanks! |
@NolaDodd, @trimakichan, and @santisecco, I want to thank you all for the feedback you left for me. I will integrate it in my next PR. |
Fixes #7647
What changes did you make?
Why did you make the changes (we will use this info to test)?
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)
Visuals before changes are applied
Visuals after changes are applied