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

Refactor markup for page templates #7795

Merged
merged 32 commits into from
Aug 16, 2024
Merged

Refactor markup for page templates #7795

merged 32 commits into from
Aug 16, 2024

Conversation

ch-tyson
Copy link
Contributor

@ch-tyson ch-tyson commented Jul 19, 2024

Summary

Refactors markup for single.html template to be more consistent.
Removes unused classes and duplicate markup.

Note

This PR should wait before merging #7803 & #7805

single.html

  • resources/single.html
  • services/single.html
  • communities/single.html
  • news/single.html
  • docs/single.html
  • events/single.html
  • _default/single.html
  • guides/single.html

Preview

Link to Preview

Solution

Refactored markup to match a consistent hierarchy for all single.html pages.

main
  article
    header
    div.in-page-nav-container
    aside.usa-in-page-nav
    div.content.usa-prose

Dev Checklist

  • PR has correct labels
  • A11y testing (voice over testing, meets WCAG, run axe tools)
  • Code is formatted properly

ch-tyson added 9 commits July 2, 2024 17:43
- Refactored resources/single.html
- Refactored services/single.html
- Refactored resources/single.html
- Refactored services/single.html

squash w before
- Refactored resources/single.html
- Refactored services/single.html
Copy link

🔍 Preview in Federalist

@ch-tyson ch-tyson self-assigned this Jul 24, 2024
@nick-mon1 nick-mon1 self-requested a review August 1, 2024 15:48
Copy link
Contributor

@nick-mon1 nick-mon1 left a comment

Choose a reason for hiding this comment

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

Suggestion

Like the updates to these pages @ch-tyson. I'm thinking with some of the clean up, we can possibly merge with @lcummings12 #7803

Fix

I fixed the prettier formatting with 9b08f82

<p>Open to the public.</p>
<section>
{{- if .Params.summary -}}
<div class="content usa-prose">
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue

For some reason on my local, I see usa-prose increasing the font-size and font-weight. Can you confirm if this is happening for you and address?

Screenshot 2024-08-01 at 11 30 20 AM

themes/digital.gov/layouts/resources/single.html Outdated Show resolved Hide resolved
themes/digital.gov/layouts/services/single.html Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used somewhere or just a draft file for what the markup structure should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used anywhere else and it was just for my convenience, should I remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

Todo

We can make a note to remove before merging if you need it for reference

@nick-mon1 nick-mon1 changed the title Refactor single and list.html templates Refactor markup for page templates Aug 5, 2024
ch-tyson and others added 5 commits August 9, 2024 12:04
- replace <aside> element with <div> for sidebar
  and simplify
- wrap main content w/ section for consistency
- simplify header structure, remove unnecessary grid containers
- remove redundant wrapper divs to streamline HTML
  structure
- minor changes to news/single and docs/single
-remove nested grid containers and rows from header
-remove unnecessary div wrappers
-consistency across all single.html
@nick-mon1 nick-mon1 requested a review from RileySeaburg August 14, 2024 12:59
Copy link
Contributor

@nick-mon1 nick-mon1 left a comment

Choose a reason for hiding this comment

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

@ch-tyson Your changes are ready for approval, I let some comments for Toni to review on event hosting.

themes/digital.gov/layouts/events/single.html Outdated Show resolved Hide resolved
Comment on lines 127 to 134
{{/* Google Stage */}}
{{- if eq .Params.event_platform "google" -}}
{{/* If is a Future Event */}}
{{- if eq $future_event true -}}
{{- .Render "stage-google" -}}
{{- end -}}
{{- end -}}

Copy link
Contributor

Choose a reason for hiding this comment

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

@ToniBonittoGSA It looks like we don't use google for future events anymore and we only have 20 files that use this option.

This code will only affect future events, we can remove it if we don't use google for event hosting.

Copy link
Member

Choose a reason for hiding this comment

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

Lets comment this out for now no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks
cc: @jdotyoon

@RileySeaburg
Copy link
Member

@nick-mon1 @lcummings12 now that the other 2 prs are merged, we have some conflicts to solve here.

@nick-mon1
Copy link
Contributor

@RileySeaburg Addressed merge conflicts

@RileySeaburg
Copy link
Member

@RileySeaburg Addressed merge conflicts
@nick-mon1
Can you approve this as well?

@RileySeaburg RileySeaburg requested a review from nick-mon1 August 16, 2024 16:43
nick-mon1
nick-mon1 previously approved these changes Aug 16, 2024
RileySeaburg
RileySeaburg previously approved these changes Aug 16, 2024
Copy link
Member

@RileySeaburg RileySeaburg left a comment

Choose a reason for hiding this comment

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

Lots of changes here. I'd like to see smaller PRs but this is great work. Thank you

@RileySeaburg RileySeaburg dismissed stale reviews from nick-mon1 and themself via 4e6a478 August 16, 2024 19:26
Copy link
Member

@RileySeaburg RileySeaburg left a comment

Choose a reason for hiding this comment

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

Previously approved thankl you @ch-tyson

@RileySeaburg RileySeaburg merged commit b298484 into main Aug 16, 2024
8 checks passed
@RileySeaburg RileySeaburg deleted the tc-refactor-main branch August 16, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants