-
-
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
img tag refactor in project leadership card #5284
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.
|
ETA: EOD Wednesday, Aug 23 |
ETA: Thursday, 8/24 |
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 @erinzz! Great work on this pull request. The commit is to and from the correct branches - it is into "hackforla:gh-pages" and from your branch of the code. The pull request contains a correctly formatted linked issue. The changes you made to the code are clean - you only removed the ending slash in the img tag for the leader-img - and the changes did not result in any visual changes on the website, as expected.
I have one small change request - can you please edit your opening comment in the pull request to remove the before and after images, and replace them with a statement that no visual changes occurred. See detailed instructions here: https://github.com/hackforla/website/blob/7f0c132c96f71230b8935759e1f8711ccb340c0f/CONTRIBUTING.md#iv-complete-pull-request-4-include-images-if-available
After you have done this, please click the circular arrows by my icon under the "reviewers" section on the top right of the pull request to re-request a review.
Thank you and please let me know if you have any questions!
Hey @allison-truhlar, thanks for reviewing! I've updated the pull request. |
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.
@erinzz - Thanks for updating the opening comment in the pull request to remove the screenshots! This pull request looks great!
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. img tag no longer includes a closing '/' as requested by the instructions. Branches are set up correctly and pull request comment has been edited to note that no visual changes are applicable. Well done!
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.
Hello @erinzz!
- You are committing into the correct branch ‘hackforla:gh-pages’
- The issue has been linked to the PR correctly
- Clean edit with slash removed from ‘leader-img’ image tag
This looks awesome and thank you for editing the PR comment.
Great job!
Review ETA: 8/24/2023 |
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.
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 @erinzz — the branching is set up correctly, the corresponding issue is linked, the requested change of removing the ending slash from the image tag on line 151 was made exactly as asked, and it didn't seem to affect any of the project leader cards for the projects I checked on my local environment.
The What and Why sections of your PR are well-written, and the Screenshots section is clear, but it's visually a bit difficult to read as the headings from the PR template were stripped away. The headings are in the PR template for a reason and should be kept in — something to keep in mind for your future PRs.
Thanks for completing this issue!
Fixes #5256
What changes did you make?
• removed slash from img tag in projects leadership card html file
Why did you make the changes (we will use this info to test)?
• for img tag notation consistency in codebase
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
No visual changes after code changes applied; see below.