-
-
Notifications
You must be signed in to change notification settings - Fork 830
Update Project Profile: 311 Data Remove Anna Kim #8057
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 Project Profile: 311 Data Remove Anna Kim #8057
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.
|
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.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Review ETA: 4/16 EOD |
Hey @Christopher-Chhim, the reviewers you had assigned are not active on the website project anymore-in general you should not assign reviewers unless they've agreed to look at your PR ahead of time. You can always post in the slack channel if you need reviews. |
Review ETA: 10 PM 4/15/25 |
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.
Good work on this pull request. There is one thing you need to change before this can be merged:
- First of all, you don't need to include your username in the branch name. You don't have to change anything here, but keep this in mind for future pull requests.
- Second, and more importantly, it seems there is an extra commit. You removed Emily Eldar from home-unite-us, when the issue we are solving is removing Anna Kim from 311 Data. It's great you want to solve both issues, but they should be in separate pull requests to keep things organized. It's possible you were trying to put them in separate PRs but accidentally used the same branch. No worries, but please undo those changes.
Otherwise, everything looks good.
Thank you for your contribution! Just clear up that one commit, and this will be ready for merging.
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 @Christopher-Chhim , thanks for tackling this issue.
Things done well
- Correct branches are used
- PR links correct issue Update Project Profile: 311 Data Remove Anna Kim #7548
- Before and after images are included and are relevant to the issue
Suggested changes
As mentioned above, a commit from another issue you worked on ( #7484 ) seems to be added to this PR. To avoid confusion, this commit should be removed from this PR.
Once these changes are made, please re-request a review from me. Thank you once again!
Availability: after 1pm (pacific) |
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.
Right on, Christopher!
Things Done Well
- The pull request done with the correct branch.
- There's a linked issue, and I understand it.
- I took a look at files changed tab and read the comments about the Emily Eldar deletion.
- I could see the changes in the browser (Thanks againg, Mugdh!)
Suggestions
I'm not sure what to suggest beyond what other's have already said.
Also ran on the browser and everything works as expected. PR approved 👍👍 |
@Christopher-Chhim I see that you made new commits, but the extra changes still appear in the "Files Changed" tab. I'm unsure whether to approve this or not -- an extra file is changed, but merging it shouldn't cause any harm. |
@TheManTheMythTheGameDev just read your comment and yeah, I see the file changes now. I probably missed it while doing my re-review. It does seem like merging this PR won't cause any issues, and I did recheck both files that were changed on my local machine to see if any unwanted changes were present, but everything does appear to be functioning normally. Based purely on functionality, this PR seems to be good, but the extra file changes definitely should be removed if able. However, I'm not entirely sure what suggestions I could make to fix this issue. |
Hi @Christopher-Chhim! Please try pulling the changes from gh-pages into your branch and push the branch again. I believe this should resolve why the changes from your old PR are showing here. Thanks! |
Hi @Christopher-Chhim Please respond to the comments above as soon as possible to let us know that you intend to complete this PR, and let us know if you need assistance. If we don't hear from you soon, we will assume that you are no longer working on this. Thanks |
Closing due to inactivity. |
Fixes #7548
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