-
-
Notifications
You must be signed in to change notification settings - Fork 844
Update list of technologies used in civic tech job webpage #8373
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 list of technologies used in civic tech job webpage #8373
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 - after 11am (pacific) |
kdaca19xx
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.
Awesome effort, MohamedTwahir!
Things Done Well
-
The pull request was done with the correct branch.
-
There's a linked issue. I read/understood it.
-
Your deletion in the Files Changed tab looks good.
-
Using Docker, I can see that "Node.js" has been removed from the Technologies list
Suggestions
- Please consider paying closer attention to the screenshots you upload to show your hard work. Your "Visuals before changes are applied" screenshot and your "Visuals after changes are applied" screenshot are the same. So, they both show Node.js in the Technologies list as if you didn't edit the Technologies variable in the code. Below is the screenshot I took while in Docker; and below that is a screenshot of what I see under your "Visuals after changes are applied".
I'd like to approve this PR, but it's probably best if you update your "Visuals after changes are applied" before you get an approval. Are you able to change the screenshot under "Visuals after changes are applied"?
|
@kdaca19xx |
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 for working on this issue.
Done Correctly
- The merge from and to branches have been correctly set.
- The issue has been correctly linked.
- The pull request title is named properly.
- The whats and whys in the pull request description have been filled out.
- The CodeQL alerts have been correctly checked off.
- Code changes have been correctly made.
- In the issue, you've correctly assigned yourself and gave your availability and ETA.
- In the issue, the project and its status has been correctly assigned.
Changes that need to be made
-
As @kdaca19xx mentioned, the screenshot for the projects webpage is the same for before and after changes are made. It looks like you captured the wrong local instance of the HackforLA website. Please upload a new screenshot for after changes are made.
Both the general projects webpage and the Civic Tech Jobs webpage uses the same markdown file for information. For an example that
civic-tech-jobs.mdfile changes both webpages, see this pull request.
|
@myronchen-git @kdaca19xx |
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 for making the corrections. I don't see anything else out of place.
Thanks again, Mohamed. I'll change my review's status to "approved". |
kdaca19xx
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.
Great job, MohamedTwahir!
Things Done Well
The pull request was done with the correct branch.
There's a linked issue. I read/understood it.
Your deletion in the Files Changed tab looks good.
Using Docker, I can see that "Node.js" has been removed from the Technologies list.
You uploaded a new screenshot under "Visuals after changes are applied" to show your hard work!
Suggestions
none.


Fixes #8330
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