-
Notifications
You must be signed in to change notification settings - Fork 196
Remove CDN to match Theme Submission Guidelines #66
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.
There are accessibility issues in these changes.
I agree, just getting the ball rolling now.
…On Fri, Aug 16, 2019 at 3:37 AM Vincent Tam ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In layouts/partials/head.html
<#66 (comment)>
:
> - <link rel="stylesheet" href="{{ "css/normalize.css" | relURL}}">
- <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Raleway:400,800,900|Source+Sans+Pro:400,700 <https://fonts.googleapis.com/css?family=Raleway:400,800,900%7CSource+Sans+Pro:400,700>">
- <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/flag-icon-css/3.1.0/css/flag-icon.min.css">
- <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/academicons/1.8.6/css/academicons.min.css">
- <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.9.0/css/all.css" integrity="sha384-i1LQnF23gykqWXg6jxC2ZbCbUMxyw5gLZY6UiUS98LYV5unm8GWmfkIS6jqJfb4E" crossorigin="anonymous">
- <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/fancybox/3.3.5/jquery.fancybox.min.css" />
- <link rel="stylesheet" href="{{ "css/main.min.css" | relURL }}">
- <link rel="stylesheet" href="{{ "css/add-on.css" | relURL }}">
- {{ else }}
- <link rel="stylesheet" href="{{ . | relURL }}">
+ {{ if .Site.Params.enableCDN }}
+ {{ if .Site.Params.highlightjs }}<link rel="stylesheet" href="//cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.9/styles/{{ <http://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.9/styles/%7B%7B> .Site.Params.highlightjsTheme | default "default" }}.min.css">{{ end }}
+ {{ range .Site.Params.cssFiles }}
+ {{ if eq . "default" }}
+ <link rel="stylesheet" href="{{ "css/normalize.css" | relURL}}">
+ <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Raleway:400,800,900|Source+Sans+Pro:400,700 <https://fonts.googleapis.com/css?family=Raleway:400,800,900%7CSource+Sans+Pro:400,700>">
Since this involves main.scss, let's get #57
<#57> done
first.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#66?email_source=notifications&email_token=ACWVJSJCCJUDYHFOPYH2TNDQEZKJ5A5CNFSM4IMDEED2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBYI37Y#discussion_r314613107>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWVJSMJUD2HOGTRJ5UIBX3QEZKJ5ANCNFSM4IMDEEDQ>
.
|
Now that I have explored Hugo Pipes (thanks @VincentTam), I think we should probably take the following approach (but want a second opinion before I start):
I don't think I missed anything, but this should really optimize everything. |
TODO:
|
I am missing Staticman files and when I have tried to rebase it has been kinda wonky. I'll let you look first. I think that is the only thing missing from this PR. |
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'm working on the slices to get Staticman JS bundled with other assets.
Since we have resources in the form of slices, the |
Curious your thoughts on having all the HLJS themes locally. Should we do that, or should we just keep that as a cdn thing (maybe keep |
Let's keep them. |
{{ $js := $jsSlice | resources.Concat "js/localbundle.js" | resources.Minify | resources.Fingerprint }} | ||
<script src="{{ $js.RelPermalink }}" integrity="{{ $js.Data.Integrity }}"> | ||
{{ else }} | ||
<script src="{{ . | relURL }}"></script> |
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.
add-on.js
is still dropped unless it is included in .Site.Params.jsFiles
.
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 back in site config file
Hey @VincentTam - I know you have been just as busy as me (I am just jumping in to clean up some issues and PRs). Any chance life has slowed down enough for you to finish? If not I can. |
…tion to explain their purpose.
5741563
to
b81ded7
Compare
I believe those adjustments complete this PR. |
With CDN, there seems to be an odd thing happening with the NavBar Title Text. Can't track down the cause. |
Description
All resources have been downloaded to the theme directory so that there isn't a reliance on CDNs.
.Site.Params.enableCDN
defaults tofalse
to adhere to guidelines. Setting totrue
enables CDNs.Motivation and Context
Closes #56 and address Theme Submission Guidelines
Screenshots (if appropriate):
Types of changes
Checklist:
theme.toml
, accordingly.