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 in-page nav #7805

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Refactor in-page nav #7805

merged 11 commits into from
Aug 16, 2024

Conversation

lcummings12
Copy link
Contributor

@lcummings12 lcummings12 commented Jul 23, 2024

Summary

Refactor in-page navigation by the creation of a "usa-in-page-nav" component which replaces redundant instances of in-page-nav.

Preview

Link to Preview

Solution

The code itself required minimal to no changes, I simply found all instances of usa-in-page-nav and replaced them with the component usa-in-page-nav. The component itself consists of the samecode that was already informally reused throughout pages.

Proposed Link Production Link
Proposed Production

Copy link

🔍 Preview in Federalist

@lcummings12 lcummings12 added the Dev: frontend ideas and issues related to the presentation layer of the site label Jul 23, 2024
RileySeaburg
RileySeaburg previously approved these changes Jul 24, 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.

Great job and thank you for the help on this!

Comment on lines 12 to 17
<div class="">
{{/* Content */}}
<div class="content usa-prose">
{{- .Content -}}
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this div with an empty class. It doesn't do anything.

Comment on lines 1 to 4
<!-- partials/core/usa-in-page-nav.html -->
<div class="usa-in-page-nav-container">
<!--
data-root-margin sets the observable "window" for the element, details here https://designsystem.digital.gov/components/in-page-navigation/#using-the-in-page-navigation-component-2
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add some comments about what this component does like how you did for #7748.

You could also move the long comment on line 4 into it to describe the settings used.

@nick-mon1 nick-mon1 mentioned this pull request Aug 1, 2024
11 tasks
@lcummings12
Copy link
Contributor Author

@nick-mon1 Added the comments and removed the extraneous div

@lcummings12 lcummings12 requested a review from nick-mon1 August 8, 2024 15:33
Comment on lines 20 to 35
<div class="usa-in-page-nav-container">
<div>
{{/* Content */}}
<div class="content usa-prose">
{{- .Content -}}
</div>

{{/* Touchpoints Form */}}
{{- partial "core/touchpoints-form.html" . -}}
<div class="content usa-prose">
{{- .Content -}}
</div>

<aside
class="dg-sidebar usa-in-page-nav"
data-title-heading-level="h4"
data-root-margin="-350px 0px -350px 0px"
aria-label="Related content"
>
<div class="dg-related-items" data-related-items>
{{/* Related Communities and Services */}}
{{- partial "core/get_related.html" . -}}
</div>
</aside>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Improvement

It would be ideal to have the component take a setting for displaying the get_related.html partial.

      <div class="usa-in-page-nav-container">
        <aside
          class="dg-sidebar usa-in-page-nav"
          data-title-heading-level="h4"
          data-root-margin="-350px 0px -350px 0px"
          aria-label="Related content"
        >
          {{ if $has_sidebar }}
          <div class="dg-related-items" data-related-items>
            {{- partial "core/get_related.html" . -}}
          </div>
          {{ end }}
        </aside>
        <div class="content usa-prose">
          {{- .Content -}}
        </div>
      </div>

Copy link
Contributor

Choose a reason for hiding this comment

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

This would entail more refactoring. We can skip this.

{{- partial "core/get_related.html" . -}}
</div>
</aside>
</div>
{{- partial "core/touchpoints-form.html" . -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix

Moved this partial call outside of the in-page nav markup, it was breaking the layout.

Screenshot 2024-08-08 at 11 55 28 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved the inline comments to the top section.

@nick-mon1 nick-mon1 self-requested a review August 13, 2024 21:31
nick-mon1
nick-mon1 previously approved these changes Aug 13, 2024
@RileySeaburg RileySeaburg merged commit beae9ed into main Aug 16, 2024
8 checks passed
@RileySeaburg RileySeaburg deleted the in-page-nav branch August 16, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev: frontend ideas and issues related to the presentation layer of the site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants