-
-
Notifications
You must be signed in to change notification settings - Fork 843
update-remove-samantha #8299
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
update-remove-samantha #8299
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: 6PM 8/28/2025 |
myronchen-git
left a comment
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.
Thank you Gabriela for working on this good first issue.
I have found the following major issues:
- Since you are removing someone from a webpage, there will be a visual change. Please run the website on your local environment again, using docker. Then compare it with the Expunge Assist webpage on the live website. Take screenshots of the change and post them on your pull request description.
I have found the following minor issues:
- On the originating issue, you should check off the action items in the issue description.
- Also on the originating issue, you should post a comment describing your availability and your ETA.
- On your pull request, the title should not have hyphens.
- On your pull request, for the reason why, it should be the underlying reason why the change is needed. You can usually get the underlying reason from the originating issue's overview.
- On your pull request, for the changes made, you should give a little bit more detail on the changes. For example, you can say that you removed Samantha Hyler from the leadership section on the Expunge Assist webpage.
santi-jose
left a comment
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 @gabbyjimenez ! Good work on this Good First Issue!
Here's what you did well:
- Into and from branches are correct
- Issue is properly linked
- CodeQL Alerts properly checked
- Changes to code match what is requested in the issue
- Changes made to code don't break the website
Changes Requested:
- I second all suggested changes by @myronchen-git
a. This issue causes a visual change. When you remove Samantha from the leadership variable in the expunge-assist.md file, there will be visible changes to the website. Specifically, a card which includes the profile picture, name, and role of Samantha will be removed from the Current Project Team section in the Expunge Assist project on the Hack For LA website. Run the development server locally using docker and take before and after screenshots to confirm the visual changes caused by the changes in the code. Then add these screenshots to the Pull Request under the Screenshots section.
b. Make sure to mark the checkboxes in the original issue as you complete them
c. Don't forget to add a comment to every issue you claim which describes your availability and completion ETA.
d. The title should not have hyphens and should be more descriptive. Create a title that gives a reader enough information to understand what the PR is for at a glance. You can use the original issue's title as a guide #8004
e. The section detailing why changes were made can be derived better from the original issue.
f. The section detailing how changes were made can be more descriptive, i.e. including that the changes made were removing Samantha Hyler from the leadership variable in the expunge-assist.md file.
Your Pull Request is almost there! These are mostly minor changes that are meant to show you the workflow for developing with Hack For LA. When you make the requested changes I'll make sure to re-review them.
|
Note to @santi-jose Please do not assign yourself to Pull Requests. When you review someone's PR, you should only be listed as one of the "Reviewers". I will unassign you from this PR and 8300 |
Thanks for letting me know! |
9a1c445 to
671fbd0
Compare
Fixes #8004
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)