Skip to content
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 Img tag refactor: citizen engagement #4732

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

oliviazhai
Copy link
Member

@oliviazhai oliviazhai commented May 26, 2023

Fixes #4449

What changes did you make and why did you make them ?

  • Remove the ending slash in the img HTML tag in citizen engagement page

there are no visual changes

@github-actions
Copy link

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.

git checkout -b oliviazhai-update-Img-Tag-Refactor gh-pages
git pull https://github.com/oliviazhai/website.git update-Img-Tag-Refactor

@github-actions github-actions bot added good first issue Good for newcomers role: front end Tasks for front end developers role: back end/devOps Tasks for back-end developers To Update ! No update has been provided Feature: Refactor HTML P-Feature: Citizen Engagement https://www.hackforla.org/citizen-engagement size: 0.25pt Can be done in 0.5 to 1.5 hours labels May 26, 2023
@LOSjr4 LOSjr4 self-requested a review May 26, 2023 17:08
Copy link
Member

@LOSjr4 LOSjr4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliviazhai Thank you for addressing the slash in this issue. Your pull request was submitted correctly. However I noticed the original issue also showed the space taken out in addition to the slash.
image
Please go back into your branch, remove the space, and commit/push again.

Also FYI, I appreciate your effort to include pictures, but regarding this issue, there wasn't any changes to show. It's important that we check Docker to make sure our changes don't break anything, but we don't always supply pictures if changes are not visible to the site.

@93Belen
Copy link
Member

93Belen commented May 28, 2023

Availability: Tuesday, Thursday and Sunday from 6am to 6pm
ETA: 5/30/23 by 6pm

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oliviazhai, I agree with the points made by @LOSjr4. Since we are aiming for a consistent formatting of HTML, please remove the extra space at the end of the tag, and also please delete the <details> and <summary> tags and simply state that there are no visual changes to the website. But aside from those minor points, the PR looks very good. Thank you!

@93Belen
Copy link
Member

93Belen commented May 29, 2023

I agree with the changes requested so I am just going to wait for that to be fix before I leave the same review :)

@oliviazhai oliviazhai requested review from roslynwythe and LOSjr4 May 30, 2023 13:10
@oliviazhai
Copy link
Member Author

Hi @oliviazhai, I agree with the points made by @LOSjr4. Since we are aiming for a consistent formatting of HTML, please remove the extra space at the end of the tag, and also please delete the <details> and <summary> tags and simply state that there are no visual changes to the website. But aside from those minor points, the PR looks very good. Thank you!

Hi,

I just did the change can you review again, thanks for the feedback

@oliviazhai
Copy link
Member Author

@oliviazhai Thank you for addressing the slash in this issue. Your pull request was submitted correctly. However I noticed the original issue also showed the space taken out in addition to the slash. image Please go back into your branch, remove the space, and commit/push again.

Also FYI, I appreciate your effort to include pictures, but regarding this issue, there wasn't any changes to show. It's important that we check Docker to make sure our changes don't break anything, but we don't always supply pictures if changes are not visible to the site.

Hi,

thanks so much for the feedback, i did the change can you review it again

Copy link
Member

@93Belen 93Belen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Great job!
You did the PR from the correct branch, linked the issue and explained what you did.
Now the img element is formatted exactly how described in the Issue.
Great job addressing the changes requested.

Copy link
Member

@LOSjr4 LOSjr4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect @oliviazhai! Thank you for going back in to remove that space and letting me know to re-review your PR.

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oliviazhai since there are no visual changes to the website, please delete the <details> and <summary> tags in the PR, along with the images, and simply state that there are no visual changes.

@github-actions github-actions bot removed the To Update ! No update has been provided label Jun 5, 2023
@oliviazhai
Copy link
Member Author

Hi @oliviazhai since there are no visual changes to the website, please delete the <details> and <summary> tags in the PR, along with the images, and simply state that there are no visual changes.

Hi, @roslynwythe, I just updated the command,thanks

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oliviazhai great job on this issue. Your code change is clean and correct and it checks out fine in the browser, you setup the branches of the PR correctly and described your work, Thank you!

@roslynwythe roslynwythe merged commit 593f698 into hackforla:gh-pages Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Refactor HTML good first issue Good for newcomers P-Feature: Citizen Engagement https://www.hackforla.org/citizen-engagement role: back end/devOps Tasks for back-end developers role: front end Tasks for front end developers size: 0.25pt Can be done in 0.5 to 1.5 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

img Tag Refactor: Citizen Engagement page
4 participants