-
Notifications
You must be signed in to change notification settings - Fork 0
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
1. Migration to 11ty, basic home page setup, broad strokes migrating styles. #2 #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Just two things of note to update.
<section class="usa-footer__primary-content usa-footer__primary-content--collapsible"> | ||
<h4 class="usa-footer__primary-link">Get in touch</h4> | ||
<ul class="usa-list usa-list--unstyled"> | ||
<li class="usa-footer__secondary-link"><a href="/contact">Customer support</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For any relative links on the site, you will want to use 11ty's url filter to properly append a possible path prefix. For all of Pages site's preview deployments, the preview is at a subpath and this url filter will automatically append it to the path. We feed the path value at a site's preview build time.
<li class="usa-footer__secondary-link"><a href="/contact">Customer support</a></li> | |
<li class="usa-footer__secondary-link"><a href="{{ '/contact' | url }}">Customer support</a></li> |
Look for any other relative links not using the url
filter and update them to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes the most sense to address this on this request all at once since a lot more content has been migrated since this PR. #9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating any of the template or includes files now will be pick up in all of the following PR's once you rebase them.
if (process.env.BASEURL) { | ||
pathPrefix = process.env.BASEURL | ||
} | ||
if (process.env.BASEURL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any documentation on it in this repo but our IDE's and linting processes are configured to save JS
indents as spaces
with the size of 2
. Update your IDE and resave the JS
files. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes the most sense to address this on this request all at once since a lot more content has been migrated since this PR. #9
Would love to see these commits merged/squased/rebased so that we don't have "WIP" in the forever-history. Thanks! |
======================================== | ||
======================================== | ||
---------------------------------------- | ||
USWDS 2.13.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I mentioned this on another PR but this settings file is for USWDS v2, but I think the package.json has USWDS v3. Can you make these match the same major version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update, good catch.
c63628c
to
5805e9f
Compare
5805e9f
to
d90bb27
Compare
Significantly decreased these except for the originating branch that's since been deleted (not using correct naming convention). |
All feedback updated in the latest peer request: #9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to still review/merge these PRs individually, thanks!
Changes proposed in this pull request:
security considerations
n/a