-
-
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
5140 img tag refactor #5295
5140 img tag refactor #5295
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.
Nice work!
- The branching was done correctly
- The what was included and descriptive
- The why was included and descriptive
- The issue was addressed correctly
- There were no changes to the visuals on the site (as it should be)
I have a few requests to clean up the PR itself. You can edit the PR by clicking the . . .
at the top right of your original comment and clicking Edit
on the menu. Please make the below changes.
- Replace
replace_this_text_with_the_issue_number
with the original issue number - Delete the unused bullet points in the what and why sections
- Delete the screenshot section and replace it with a note that there were no visual changes
- Add any labels that are missing to match the labels on the original issue (this usually happens automatically, but because there was no issue number it didn't work)
I also see a bit of white space was deleted in addition to the image tag change in the code. Generally, we don't want to see any additional changes, but in this case because it is small change in white space, it's okay to leave it as is. Just something to note for the future. The changes the PR is requesting can be reviewed by clicking the files changed tab in the PR.
When you're done with the changes you can just click the circle with the arrows next to my name in the reviewers section to re-request a review from me.
Great work figuring out our git workflow! And thanks for taking the time to contribute to the site!
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 @bluechocolate2019! The changes look great. Thank you for taking the time to make them and for contributing!
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 @bluechocolate2019 — the branching is set up correctly, the corresponding issue has been linked, the requested change of removing the ending slash from the image tag on only line 47 has been made, and doing so did not seem to affect any of the mini project card images on the Program Areas page.
An extra whitespace character was deleted on line 85 which doesn't affect the page visually or functionally, but you may want to check your linter rules (if you are using one) while working within HfLA's codebase for your future PRs, as only line 47 was expected to be changed. For this instance, this whitespace removal wasn't too impactful, but in future, more complex PRs you make where more files are being touched, unaccounted removals that are committed could be troublesome and problematic to work with when merging.
The What and Why sections of your PR are well-written, and the Screenshots section is clear after deleting the excess summary and details tags. I also appreciate that you took the time to revise your initial PR based on reviewer feedback!
Thank you for taking up this issue! 🙌🏼
Fixes #5140
What changes did you make?
Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
there were no changes to the visuals on the site