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
Remove jekyll assets #314
Remove jekyll assets #314
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.
Added a couple of a suggestions.
_includes/footer.html
Outdated
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.
Do you think it's worth wrapping {{site.baseurl}}/assets/uswds/
in a utility of some sort? This gets repeated a lot.
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.
Or at least {{site.baseurl}}/assets/
, which gets repeated even more?
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.
the first tag can also be replaced with {{ 'assets/uswds/mypath.ext' | relative_url }}
but that's not shorter (or for me any easier to read). I'll include this as an option when finishing the migration guide.
In general, I'd rather leave as is because it is the filepath, and making it shorter for DRY sake actually makes it harder to tell where the file is (which is part of the problem with jekyll-assets
)
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.
OK, that's fair. Thanks, Drew.
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.
Looks good to me!
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.
Looks like a lot of the asset images are broken in the preview build
The asset paths are like @drewbo, should |
2c9b83d
to
10a193e
Compare
Co-authored-by: Sven Aas <12150+svenaas@users.noreply.github.com>
431702e
to
1054204
Compare
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.
Changes look good, as does the preview build.
32b4453
to
fe01bd6
Compare
Changes proposed in this pull request:
jekyll-assets
#186:Notes
Security considerations
Removes unmaintained software and allows for moving off EOL Ruby 2.7