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

Fix glossary header bug #7371

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Fix glossary header bug #7371

merged 10 commits into from
Mar 26, 2024

Conversation

nick-mon1
Copy link
Contributor

@nick-mon1 nick-mon1 commented Mar 1, 2024

Summary

Guide pages that display a summary-box component also display the Glossary h2 when it should not be visible.
glossary.html is unnecessarily rendered on guide pages that don't use a glossary.

See guides/dap/common-questions-about-dap
Screen Shot 2024-03-01 at 2 08 39 PM

Preview

Link to Preview

Solution

  1. Add a conditional check to display glossary.html when .Parent.Params.glossary is true.
  2. If the summary-box component is used on a page with a glossary component enabled, then don't display the Glossary h2.
  3. Hide the Glossary h2 in summary-box.js

How To Test

  1. Visit guide page and should not see glossary header in summary box when glossary is not enabled.
  2. Visit guide page and should not see glossary header in summary box when glossary is enabled.
    • set summary_box: true on content/guides/hcd/discovery-concepts/present.md on your local machine
  3. Visit guide landing page and should not see glossary header in summary box when glossary is enabled.
    • set summary_box: true on content/guides/hcd/introduction/_index.md on your local machine

What To Test

  • glossary.html is not rendered when glossary field is not set
    • dg-glossary__container is not present in markup
  • summary-box component does not display Glossary h2 at all (even when set).

Dev Checklist

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

- [x] tested when glossary is not enabled
- [x] tested when glossary is enabled with summary box
- [x] updated glossary.html to set header class for tagetting to hide in
  summary-box.js
- [x] set conditional to hide/display glossary.html partial based on
  glossary field set in parent _index.md
@nick-mon1 nick-mon1 self-assigned this Mar 1, 2024
@nick-mon1 nick-mon1 added bug Things that are not working as they should be Dev: Template Logic Fixes and improvements associated with our static site generator labels Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

🔍 Preview in Federalist

Comment on lines +22 to +24
{{- if .Parent.Params.glossary -}}
{{- partial "core/glossary.html" . -}}
{{- end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if .Parent page has glossary field set. This refers to the root _index.md in the guide directory.

@@ -13,7 +13,7 @@
</svg>
</button>

<h2>Glossary</h2>
<h2 class="dg-glossary__header">Glossary</h2>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added class as a hook for the javascript.

@@ -10,7 +10,7 @@

const guideSummaryList = guideSummary.querySelector(".usa-list");
const pageHeaders = document.querySelectorAll(
"h2:not(.usa-summary-box__heading, .dg-guide__content-header-title)"
"h2:not(.usa-summary-box__heading, .dg-guide__content-header-title, .dg-glossary__header)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This query acts as "blacklist" of headers to not in the summary box component depending on the page it is called from.

@nick-mon1 nick-mon1 marked this pull request as draft March 1, 2024 19:06
@nick-mon1 nick-mon1 requested review from mejiaj and clmedders March 1, 2024 19:06
@nick-mon1

This comment was marked as resolved.

@nick-mon1 nick-mon1 marked this pull request as ready for review March 1, 2024 19:16
@nick-mon1 nick-mon1 changed the title Fix glossary bug Fix glossary header bug Mar 1, 2024
clmedders
clmedders previously approved these changes Mar 6, 2024
Copy link
Contributor

@clmedders clmedders left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Works well, nice job. Minor comments on adding documentation.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Looks good overall. Added suggestions for clarity & code comments.

themes/digital.gov/layouts/partials/core/glossary.html Outdated Show resolved Hide resolved
themes/digital.gov/layouts/partials/core/glossary.html Outdated Show resolved Hide resolved
themes/digital.gov/src/js/summary-box.js Outdated Show resolved Hide resolved
@nick-mon1 nick-mon1 requested a review from mejiaj March 15, 2024 20:16
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

LGTM, removed duplicate header in c993905.

@ToniBonittoGSA ToniBonittoGSA merged commit 156b0e0 into main Mar 26, 2024
4 checks passed
@ToniBonittoGSA ToniBonittoGSA deleted the nl-fix-glossary-bug branch March 26, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that are not working as they should be Dev: Template Logic Fixes and improvements associated with our static site generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants