-
-
Notifications
You must be signed in to change notification settings - Fork 786
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: LA TDM Calculator Add Parisa Jannatifard #7643 #7706
Update Project Profile: LA TDM Calculator Add Parisa Jannatifard #7643 #7706
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: 11/6/2024 Wednesday |
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, @Sk-223 !
- The branch you made the pull request from is correct
- The linked issue number is included
- The changes are correctly reflected on the LA TDM Calculator projects page
Improvements:
- Name the branch with more specific details, such as
LA-TDM-calculator-add-parisa-jannatifard-7643
- Provide more details to
What changes did you make?
andWhy did you make the changes (we will use this info to test)?
. Usually, the original issue should give some context for this.
Changes you need to make:
- Address the questions posted by
HackforLABot
in the comments section of the original issue - Complete the
CodeQL Alerts
section - Provide the before-and-after screenshots of the website view instead of the code
Please re-request me to review and approve this PR once you make the changes. Feel free to reach out if you have any questions.
Hi @siyunfeng! Thank you for taking the time to review my request! |
Thanks for updating your availability and ETA in the original issue, @Sk-223 . Please fix your Docker issue so you can verify the changes you made by viewing the LA TDM calculator page in your local environment and the Hack for LA website. It's required from the original issue. I don't see the changes to the branch name or the response to The Please check the boxes in |
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.
Nice job making the changes, @Sk-223 !
- Availability and ETA are posted in the original issue
CodeQL Alerts
check is completed- Before-and-after screenshots are modified and match the change
Thank you for your contribution! Keep it up!
…update-project-profile-LA-TDM-Calculator-7643
Review ETA: 11/7/2024 Wednesday |
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, @Sk-223 !
• The branch you made the pull request from is correct
• The linked issue number is included
Improvements: The changes match the requested changes from the Issue #7643 , however, the member ID for Parisa that was provided on the issue is incorrect. Upon searching for Parisa Jannatifard on Slack, their member ID is actually U05Q5D34ARZ and therefore, the slack link should be: https://hackforla.slack.com/team/U05Q5D34ARZ
This will need to be adjusted on not only your PR, but possibly on the Issue itself. @t-will-gillis , can you advise if Issue #7643 needs to be corrected to provide the correct slack link?
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.
@Sk-223 Great job on this issue! I agree with the comment above. It does seem like the incorrect link was listed in the issue. Everything else looks good to me.
Hi @floydferrer! Thank you for taking the time to review this PR for me. Great catch on the ID. I've edited my PR to reflect the correct slack ID to U05Q5D34ARZ. |
Thank you @A-Wu5! |
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 updating the correct Slack member ID, @Sk-223 !
The URLs work and everything looks good.
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.
@Sk-223 Thank you for taking on this issue.
- The screenshots are helpful.
- The branch name is correct.
- The description is good.
Great job. Keep it up !
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 your help @floydferrer! I've flagged the discrepancy in the issue request. I do have one more question. Would you also be able to help me understand this next step?
Where is the Dependency section in this issue to check off after adding Parisa to the leadership section? It also looks like the initiating ER is already in the Questions / In Review column, but the Dependency label has been re-added to the issue. |
Hey @Sk-223 Great job! I want to note that you placed Parisa's Product Manager profile exactly where it should be: after the other Product Manager Ebi. I don't think the issue indicated this specifically so thank you for choosing the appropriate placement. Also kudos to @floydferrer for noticing that the Slack link needed to be corrected and @siyunfeng for the thorough comments! |
The note about checking off the Dependency does not apply on this issue, so you can ignore this. The reference really means that if all of the issues shown here are completed, then remove the |
Fixes #7643
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