-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
docs: bundle assets with Hugo #38993
Conversation
2ac7d3f
to
0a4e3a3
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.
Poked around locally, looks great!
0a4e3a3
to
9ffdab6
Compare
This needs a little love regarding StackBlitz changes, so I'm going to hold off a bit for now. :) Also, I couldn't make |
1abfd22
to
ae59a6c
Compare
e69ca90
to
a91c27f
Compare
2763174
to
b68e1cc
Compare
521b089
to
9ec51cd
Compare
Quickly tried something to move the search part to its own CSS file as suggested in the description's TODO list via bea1430 Feel free to drop this commit if it's not what you had in mind. https://deploy-preview-38993--twbs-bootstrap.netlify.app//docs/5.3/assets/css/search.css contains some duplicate code from |
eacffae
to
bfd9de1
Compare
e6f9a98
to
2b99c57
Compare
7fecb1a
to
4a5b848
Compare
b7b48b3
to
e28541b
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.
Great PR! Few things in comments to tackle and I think we're good to go!
I have mixed feelings about the split of docs.css
, one one hand it's clearer and StackBlitz can embed only this file without the search part, on the other hand, search.css
and docs.css
have some duplicate code:
:root,
[data-bs-theme="light"] {
--bd-purple: #4c0bce;
--bd-violet: #712cf9;
--bd-accent: #ffe484;
--bd-violet-rgb: 112.520718, 44.062154, 249.437846;
--bd-accent-rgb: 255, 228, 132;
--bd-pink-rgb: 214, 51, 132;
--bd-teal-rgb: 32, 201, 151;
--bd-violet-bg: var(--bd-violet);
--bd-toc-color: var(--bd-violet);
--bd-sidebar-link-bg: rgba(var(--bd-violet-rgb), 0.1);
--bd-callout-link: 10, 88, 202;
--bd-callout-code-color: #ab296a;
--bd-pre-bg: var(--bs-tertiary-bg);
}
[data-bs-theme="dark"] {
--bd-violet: #9461fb;
--bd-violet-bg: #712cf9;
--bd-toc-color: var(--bs-emphasis-color);
--bd-sidebar-link-bg: rgba(84, 33, 187, 0.5);
--bd-callout-link: 110, 168, 254;
--bd-callout-code-color: #e685b5;
--bd-pre-bg: #1b1f22;
But I don't have a strong opinion, so let's try it :)
However, I'm not sure if it's needed, but the generated search.css
doesn't have any legal header like docs.css
; something like:
/*!
* Bootstrap Docs (https://getbootstrap.com/)
* Copyright 2011-2024 The Bootstrap Authors
* Licensed under the Creative Commons Attribution 3.0 Unported License.
* For details, see https://creativecommons.org/licenses/by/3.0/.
*/
site/layouts/partials/scripts.html
Outdated
"cssCdn" .Site.Params.cdn.css | ||
"jsBundleCdn" .Site.Params.cdn.js_bundle | ||
"docsVersion" .Site.Params.docs_version | ||
"jsSnippetFile" (os.ReadFile "site/assets/js/snippets.js") |
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.
Isn't it "site/assets/js/partials/snippets.js"?
IDK if it's linked directly, but when going to https://deploy-preview-38993--twbs-bootstrap.netlify.app/docs/5.3/components/tooltips/#tooltips-on-links and click on the StackBlitz button, the index.js
is empty, and locally it's not.
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.
This needs more debugging.
I'll try to fix it but if not I'll just drop the whole snippetFile. I'm generally not a fan of complicating things and if this means some examples don't have a StackBlitz, that's OK to me compared to the previous situation.
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.
BTW any help is welcome here. It seems we were eval
ing the script before, not sure how to keep the same behavior.
Don't get too fixated on the license headers, it's useless but in your patch you missed to add it 😛 As for the duplicate vars, I don't care too much. I'd like them deduplicated, but I can live with it because the gains are bigger. The rest should be fixed with the latest pushes :) |
a1c0980
to
3206c49
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.
Your latest modifications fix the issues I encountered during my first tests. There are a lot of use cases, I've tested locally and via Netlify deployment only the trickiest; and it works well!
Neat! LGTM!
Also: * load any docs' third-party dependencies from node_modules (except for examples) * exclude docsearch from layouts that don't use it * preconnect to algolia only when not examples layout * switch to `RelPermalink` * refactor JS assets into partials
* tweak indentation * stop extending the sdk object * conditionally add `index.js`
712922b
to
b159be5
Compare
Description
Motivation & Context
Type of changes
Checklist
npm run lint
)Live previews
Related issues
Fixes #27412 (only some of the examples are still using libraries from the CDN)
Note that at some point we need to switch to dart sass for docs, but I couldn't make it to work and it requires the manual installation of Dart Sass, see #39700.
TODO:
@docsearch/css
in docs or search.cssFor reviewers: