Layouts and menu #20

jamesthebrooks · 2024-10-24T15:48:06Z

Changes proposed in this pull request:

Layout files migrated over
Main menu shows default items as data is migrated in another branch

sknep

Don't be afraid to re-name or reorganize some variables to make this nav stuff less annoying to understand

sknep · 2024-10-29T15:40:13Z

_includes/collection-item.html

@@ -1,28 +1,33 @@
-<li class="usa-collection__item">
+<div class="usa-prose padding-right-4">


does the name of this component still make sense if you're pulling the collection item class out? also if these are items in a list, they should still be <li>s

This change was mostly to keep the look and feel of the existing cg-site--I'll defer to you on this--pages way or pre-existing cg-site look? They probably shouldn't be in a list as these are multi-part paginated code blocks. Semantically, it's fine either way.

sknep · 2024-10-29T15:41:27Z

_includes/doc-registry.html

@@ -0,0 +1,31 @@
+{%- assign documentation_navigation=site.documentation_navigation -%}


just curious, why the assignment? you can loop thru site.documentation_navigation on the next line without assigning it.

No idea. Not my change, but is a 1:1 from the old site. I'm updating to remove the assignment, but not actually sure where this template is used off the top of my head.

sknep · 2024-10-29T15:41:56Z

_includes/doc-registry.html

+{%- assign documentation_navigation=site.documentation_navigation -%}
+{%- for nav_item in documentation_navigation -%}
+  <h2> {%- if nav_item.icon -%}
+  <i class='fa fa-fw {{ nav_item.icon }}'></i>


these are the font awesome icon classes. Was it not possible to migrate these to USWDS icons?

I don't even see a reference to this template. Deleting the file.

sknep · 2024-10-29T15:42:43Z

_includes/footer.html

-                <a href="https://www.gsa.gov">
-                    {% image_with_class "_img/gsa-logo.svg" "usa-identifier__logo" "GSA logo identifier" %}
-                </a>
+    <div class="footer-section-bottom usa-footer__primary-section bg-accent-warm-light border-top border-accent-warm-dark">


can you tell me why you went full utility classes here instead of sticking with the identifier classes? I can't tell what's better about this implementation.

sknep · 2024-10-29T15:44:42Z

_includes/footer.html

+                            <li class="usa-footer__secondary-link"><a href="/contact">Customer support</a></li>
+                            <li class="usa-footer__secondary-link"><a href="mailto:[email protected]">Business
+                                inquiries</a></li>
+                            <li class="usa-footer__secondary-link"><a href="https://join.tts.gsa.gov/">Join our team</a>


IMO all external (and mailto) links should open in a new window. Also, there's no way you could be familiar with this already, but here's some guidance: https://github.com/cloud-gov/product/blob/main/ContentGuide.md#2-referring-to-external-or-non-government-entities

also join.tts.gsa.gov recently moved to https://tts.gsa.gov/join/

I updated this to override 11ty's default URL filter and am using it {{ somelink | url }} in the template here going forward. Not ready to merge yet.

sknep · 2024-10-29T15:51:12Z

_includes/layouts/base.html

+    {% include "menu.html" primary_navigation: pages.navigation.primary secondary_navigation: pages.navigation.secondary %}
+  {% else %}
+      {% include "menu.html" primary_navigation: site.primary_navigation  secondary_navigation: site.secondary_navigation %}
+  {% endcase %}


we use assign in so many places, it seems obv to do it here too. would something like this work? note that you'd need to rename the final keys for one of them (primary -> primary_navigation or vice-vera )

{% assign nav = site %} {% if "pages" %} {% nav = pages.navigation %} {% endif %} {% include "menu.html" primary_navigation: nav.primary secondary_navigation: nav.secondary %}

sknep · 2024-10-29T15:56:04Z

_includes/layouts/docs.html

+                {% assign toc_content = content | toc | strip | strip_newlines %}
+                {% assign empty_toc_content = '<ol id="toc" class="section-nav"></ol>' %}
+
+                {% if showtoc != false and toc_content != '' and toc_content != empty_toc_content %}


any way to simplify this logic? seems messy

Simplified the logic by moving it to eleventy.js. Compiles fine, but won't be able to feature test until both this and documentation is merged in (PR not yet created).

_includes/layouts/docs.html

sknep · 2024-10-29T15:57:31Z

_includes/layouts/page.html

@@ -6,15 +6,31 @@
 This template is for a single page that does not have a date associated with it. For example, an about page.
 {% endcomment %}

-<div id="main-content" class="usa-layout-docs usa-section">
+<div class="usa-layout-docs usa-section {{background}} page-layout">


what's background for, or where is it being set? if it's a color, wouldn't it be bg-{{color}}-{{shade}} like .bg-primary-dark?

It comes from any given page's front matter so you'd set it there to whatever makes sense.

Example- some-page.html:
background: bg-primary-dark

It's not a convention I came up with... it's either from cg-site or pages-11ty (can't remember).

sknep · 2024-10-29T15:57:56Z

_includes/layouts/post.html

+
+        <div class="grid-row">
+          <div class="grid-col flex-4"></div>
+          <div class="grid-col flex-1"><p><a href="{{ site.github_url }}/edit/{{ site.github_branch }}/{{ page.relative_path }}">Suggest edits</a></p></div>


new window!

See .eleventy.js

I don't follow -- can you explain?

I think you already saw it by now because of another comment, but here: https://github.com/cloud-gov/site/pull/20/files#diff-c306e0a99961a16f5c5c83996caa0958b94006d97f97475049ea3a08036bb5b0

Eleventy now handles external links on all content and templates during the build through a custom function on the markdown-it plugin.

.eleventy.js has custom code for external links processing now, but I think you already saw that and this comment was earlier.

Eleventy now handles external links on all content and templates during the build through a custom function on the markdown-it plugin.

Hmm... That would work on markdown file content parsed by the markdown engine. This part is in a template. Are you sure it still works that way?

…needed

sknep · 2024-11-04T15:38:43Z

.eleventy.js

+  });
+
+  config.addFilter("shouldShowTOC", (content, showToc) => {
+    return !!showToc && content !== "" && content !== '<ol id="toc" class="section-nav"></ol>';


For every new function/filter you add to the eleventy config, do me a solid and comment what it's for. I can kinda tell but it's always nice to have a little more what/why <3

In another comment on the docs.html page, you asked for the template markup to be simpler and so I moved the logic to here. The new docs.html code looks like this now:

{% assign toc_content = content | prepTOCContent %} {% if toc_content | shouldShowTOC: showtoc %} <h2>Table of Contents</h2> <div id="table-of-contents"> {{ toc_content }} </div> {% endif %}

The markup is simpler now, but the logic still needs to exist somewhere.

If you're going to add custom functions/filters (magic) to eleventy, it needs to be commented in place for other people to understand what it is and how it works. Maybe you thought I meant here on the PR?

sknep · 2024-11-04T15:40:52Z

.eleventy.js

+        const token = tokens[idx];
+        const hrefAttr = token.attrs ? token.attrs.find(attr => attr[0] === 'href') : null;
+        if (hrefAttr && /^https?:\/\//.test(hrefAttr[1])) {
+          token.attrSet('target', '_blank');


this is awesome and makes me happy. If you also add a class ("usa-link--external") I think we get the little visual indicator (docs) which would be nice in the article content.

sknep · 2024-11-04T15:41:52Z

.eleventy.js

+      const dom = new JSDOM(content);
+      const links = dom.window.document.querySelectorAll("a[href^='http']");
+
+      links.forEach(link => {


oh maybe this is where to put the external link class? not the above one?

Should be both. One is for markdown and the other is for html files.

Oh. I understand the markdown one but I think doing it for HTML templates might be too heavy-handed overall. Rather than dynamically re-parsing the DOM during the build, i'd rather we just added the _blank etc when we code the HTML.

sknep · 2024-11-04T15:43:20Z

_includes/layouts/page.html

+
+        <div class="grid-row">
+          <div class="grid-col flex-4"></div>
+          <div class="grid-col flex-1"><p><a href="{{ site.github_url }}/edit/{{ site.github_branch }}/{{ page.relative_path }}">Suggest edits</a></p></div>


can this one be in a new window too?

It would be automatically from the 11ty build, yes.

sknep · 2024-11-04T15:45:16Z

js/app.js

@@ -48,3 +48,27 @@ const sideNavExpansion = function (event) {
 for (let i = 0; i < sideNavParents.length; i++) {
  sideNavParents[i].addEventListener('click', sideNavExpansion, false);
 }
+
+


Can this be done more simply, with a parent class that handles the toggling? this is a lot of dom manipulation for what is effectively a few CSS changes.

Probably, but if I make changes here I can't test them as the sidenav doesn't exist in this branch. We would have to wait until the PR for branch chore-dynamic-sidenav is up for review--that branch and PR does not exist yet. This block is a 1:1 from the Jekyll site with minimal changes to make it continue to function. Please let me know how you'd like me to proceed.

sknep · 2024-11-04T16:37:14Z

Something's broken in this build, see the build logs:


2024-10-31 15:23:23 INFO [run-federalist-script] [11ty] Original error stack trace: expected "|" before filter, file:/tmp/work/site_repo/_includes/footer.html, line:16, col:55

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]    14|                                 </li>

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]    15|                                 <li class="usa-footer__secondary-link">

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty] >> 16|                                     <a href="{{ mailto:[email protected] | url }}">Business inquiries</a>

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]                                                              ^

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]    17|                                 </li>

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]    18|                                 <li class="usa-footer__secondary-link">

2024-10-31 15:23:23 INFO [run-federalist-script] [11ty]    19|                                     <a href="{{ 'https://join.tts.gsa.gov/' | url }}">Join our team</a>

2024-10-31 15:23:23 ERROR [main] There was a problem running the federalist script, see the above logs for details.

…d html files

sknep · 2024-11-05T18:06:30Z

Is the build still broken? I think that mailto link might not work with your custom HTML link fixer

jamesthebrooks requested review from apburnes and sknep October 24, 2024 17:04

jamesthebrooks force-pushed the chore-layouts-and-menu branch from 9718674 to 5aa53f0 Compare October 28, 2024 15:31

sknep requested changes Oct 29, 2024

View reviewed changes

jamesthebrooks force-pushed the chore-layouts-and-menu branch 2 times, most recently from 4bda9e7 to 1ea05ac Compare October 30, 2024 17:01

jamesthebrooks added 2 commits October 30, 2024 13:30

layouts and main menu

291a0a7

removed unused layout

c6a2f6a

jamesthebrooks force-pushed the chore-layouts-and-menu branch from 1ea05ac to c6a2f6a Compare October 30, 2024 17:31

jamesthebrooks added 7 commits October 30, 2024 13:50

layouts branch revealed code required in app.js previously thought un…

dacb963

…needed

verifying footer.html migration from cg-site

92d76db

a cleaner way to handle external urls

1603df2

rewrite of base.html to incorporate PR comments

b2fb80f

handling added for external links globally

3a8ac86

simplified template logic for showing the toc

5501836

no longer needed |url for external links

5feeb24

jamesthebrooks requested a review from sknep November 1, 2024 13:07

sknep reviewed Nov 4, 2024

View reviewed changes

jamesthebrooks added 2 commits November 5, 2024 09:22

added class usa-link--external to external links processing for md an…

a8c0e09

…d html files

added comments for custom 11ty filters

0a202f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layouts and menu #20

Layouts and menu #20

jamesthebrooks commented Oct 24, 2024

sknep left a comment

sknep Oct 29, 2024

jamesthebrooks Oct 29, 2024

sknep Oct 29, 2024

jamesthebrooks Oct 29, 2024

sknep Oct 29, 2024

jamesthebrooks Oct 29, 2024

sknep Oct 29, 2024

sknep Oct 29, 2024

jamesthebrooks Oct 30, 2024

sknep Oct 29, 2024

sknep Oct 29, 2024

jamesthebrooks Oct 31, 2024

sknep Oct 29, 2024

jamesthebrooks Nov 1, 2024 •

edited

Loading

sknep Oct 29, 2024

jamesthebrooks Nov 1, 2024

sknep Nov 4, 2024

jamesthebrooks Nov 5, 2024

jamesthebrooks Nov 5, 2024

sknep Nov 5, 2024

sknep Nov 4, 2024

jamesthebrooks Nov 5, 2024

sknep Nov 5, 2024

sknep Nov 4, 2024 •

edited

Loading

sknep Nov 4, 2024

jamesthebrooks Nov 5, 2024

sknep Nov 5, 2024

sknep Nov 4, 2024

jamesthebrooks Nov 5, 2024

sknep Nov 4, 2024

jamesthebrooks Nov 5, 2024

sknep commented Nov 4, 2024

sknep commented Nov 5, 2024

		@@ -1,28 +1,33 @@
		<li class="usa-collection__item">
		<div class="usa-prose padding-right-4">

		@@ -0,0 +1,31 @@
		{%- assign documentation_navigation=site.documentation_navigation -%}

Layouts and menu #20

Are you sure you want to change the base?

Layouts and menu #20

Conversation

jamesthebrooks commented Oct 24, 2024

Changes proposed in this pull request:

sknep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesthebrooks Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknep Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknep commented Nov 4, 2024

sknep commented Nov 5, 2024

jamesthebrooks Nov 1, 2024 •

edited

Loading

sknep Nov 4, 2024 •

edited

Loading