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.

Sign up for GitHub

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

Jump to bottom

Refactor HTML to Resolve Linter Error #5434

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Refactor HTML to Resolve Linter Error #5434

merged 3 commits into from
Sep 19, 2023

Conversation

efranzener
Copy link
Member

Fixes #5286

What changes did you make?

  • Updated the value of attribute [class] to be in double quotes.

Why did you make the changes (we will use this info to test)?

  • To resolve the linter error "Value of attribute [class] must be in double quotes".

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

  • No visual changes to the website.

@ronaldpaek ronaldpaek self-requested a review September 4, 2023 13:18
@ronaldpaek
Copy link
Member

ronaldpaek commented Sep 4, 2023 •
edited
Loading

I plan to review this issue on Thursday, Sept 7th.

@LRenDO LRenDO self-requested a review September 7, 2023 05:50
LRenDO
LRenDO previously requested changes Sep 7, 2023
View reviewed changes
Copy link
Member

@LRenDO LRenDO left a comment •
edited by efranzener
Loading

Choose a reason for hiding this comment

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

Hi @efranzener!

Great job with this pull request!

  • The branching was done correctly
  • Issue number was listed
  • The quotes were changed correctly
  • The PR request clearly states what was updated
  • The PR request states why the changes are being made

I do have a few requests. Please make the following changes.

  • Delete the /after alt="" on line 3. It's likely that change was merged after you started working on the issue.
  • Delete line 1 refactor-html-fix-linter-error-5286. Only the changes listed in the issue should be made.
  • Add the labels that are on the original issue to this PR request (usually this happens automatically, but sometimes it doesn't work. It generally fails when the issue number isn't added the when the PR is created.)

Once you've pushed the changes, you can click the circle with the arrows next to my name in the reviewers section to request a review of changes.

Thanks for taking the time to contribute to the website!

ronaldpaek
ronaldpaek suggested changes Sep 9, 2023
View reviewed changes
Copy link
Member

@ronaldpaek ronaldpaek left a comment

Choose a reason for hiding this comment

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

Hi good job fixing what the increase says to fix.

I also agree you should remove the text refactor-html-fix-linter-error-5286 at the top of the file.

Oh I understand now, we had other issues removing the forward slash on img tag so if you could leave the img tag in it's original please.

Other than that fixing those issues and also adding the labels on the issue like Ren mentioned and you're good to go! :)

@efranzener
Copy link
Member Author

efranzener commented Sep 9, 2023 •
edited
Loading

Availability for this week: Monday - Friday: 3pm-5pm
My estimated ETA for completing this issue: Tuesday 9/12, by 9pm

@efranzener efranzener added good first issue Good for newcomers role: front end Tasks for front end developers role: back end/devOps Tasks for back-end developers P-Feature: About Us https://www.hackforla.org/about/ Feature: Refactor HTML size: 0.25pt Can be done in 0.5 to 1.5 hours labels Sep 13, 2023
@efranzener
Copy link
Member Author

Hi @LRenDO and @ronaldpaek!

Thank you both for your feedbacks.
As per your request, I applied the following changes:

  • Deleted the / after alt="" on line 3.
  • Deleted line 1 refactor-html-fix-linter-error-5286
  • Added the labels from the original issue to this pull request.

Please let me know if there is any additional changes to be made.

ronaldpaek
ronaldpaek approved these changes Sep 13, 2023
View reviewed changes
Copy link
Member

@ronaldpaek ronaldpaek left a comment

Choose a reason for hiding this comment

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

Everything looks correct to me. Great job!

@t-will-gillis t-will-gillis self-requested a review September 19, 2023 21:07
t-will-gillis
t-will-gillis approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @efranzener Great job- thanks for working with @ronaldpaek and @LRenDO to make the changes to the initial PR- everything looks good!

@t-will-gillis t-will-gillis dismissed LRenDO’s stale review September 19, 2023 21:11

The requested changes were made- thanks!

@t-will-gillis t-will-gillis merged commit f7151b3 into hackforla:gh-pages Sep 19, 2023
@efranzener efranzener deleted the refactor-html-fix-linter-error-5286 branch October 1, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldpaek ronaldpaek ronaldpaek approved these changes

@t-will-gillis t-will-gillis t-will-gillis approved these changes

@LRenDO LRenDO Awaiting requested review from LRenDO

Assignees
No one assigned
Labels
Feature: Refactor HTML good first issue Good for newcomers P-Feature: About Us https://www.hackforla.org/about/ 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor HTML to resolve linter error: _includes/about-page/about-card-metrics.html
4 participants
@efranzener @ronaldpaek @t-will-gillis @LRenDO