-
-
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
Refactored img tag events page right col #5186 #5245
Refactored img tag events page right col #5186 #5245
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: Afternoons |
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 Erin,
The ending slash was removed correctly. You correctly documented the changes, and since there were no visual changes, you stated that correctly. I ran the site locally to confirm everything works as it should. Everything looks good in that regard. One suggestion that I might make is that under the "Why did you make the changes (we will use this info to test)?" section of the documentation, there is usually a reason for these changes to be made. In this case, the answer can be found within the issue itself:
Overview
We want to change an img HTML tag ending with a slash (<img.../>) to an img tag without an ending slash (<img...>) so that the codebase is consistent with how we use img HTML tags.
Other than that, everything else looks good, great job!
Hey Dorian, thank you for your feedback! Great tip that I will note for future issues! |
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, and the change of removing the ending slash from the image tag on line 2 has been made (without any other changes) as requested. When testing your branch on my local environment, the Events page seems to be visually and functionally unaffected by this change (as expected). Good job!
Before this gets merged, there's a couple of changes that I think should be edited in the PR itself:
- In the Why section, I would say that @DorianDeptuch's suggestion would be more meaningful to add and take note of for documentation purposes, instead of explaining that ending slashes in image tags are optional.
- It would be easier to see that no visual changes have been made if they weren't inside the
<details>
tag — deleting the<details>
and<summary>
tags and just noting 'No visual changes to the website.` underneath the header would be better.
Once you make these changes, let me know and I'll merge this PR.
Edit: There's also no need to assign yourself to your own pull request - I unassigned you already, but just a light FYI.
Hey @adrianang, thank you for feedback and the clear instructions! I've updated the pull request documentation. |
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 Thank you for making those changes! It looks good to me, and everything else is still set up correctly and unchanged from my last review.
Thanks for taking up this issue! 🙌🏼
Fixes #5186
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 are no visual changes made to the website.