-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
feat: make published html scripts configurable #11149
base: master
Are you sure you want to change the base?
Conversation
@andelf @logseq-cldwalker Hi guys, just notice that this PR exists for some time. I think it is a good feature for those who are using logseq to publish their blog. Could you please review it or leave some comments when you are free? |
@CNLHC Hi. Sure. Could you provide a specific example of how you're using the new option(s)? I'll take a look at this next week |
Ok. I think two common use cases are:
With this PR and logseq/publish-spa#27 merged, a user can define a I will post an real example later. |
Use case 1 : integrate with Google Analytics
;; config.edn
:html-options {:scripts [{:src "https://www.googletagmanager.com/gtag/js?id=G-XXXXXXXX"},
{:content "
window.dataLayer = window.dataLayer || [];
function gtag(){dataLayer.push(arguments);}
gtag('js', new Date());
gtag('config', 'G-XXXXXXXX');"}]}
logseq-publish-spa <OUTPUT> -s <STATIC> -d <INPUT> --theme-mode dark --accent-color indigo
<!-- these two scripts were generated from config.edn -->
<script src="https://www.googletagmanager.com/gtag/js?id=G-XXXXXXXX"></script>
<script> window.dataLayer = window.dataLayer || [];
function gtag() { dataLayer.push(arguments); }
gtag('js', new Date());
gtag('config', 'G-XXXXXXXX');</script>
<!-- end -->
<script src="static/js/react.production.min.js"></script>
<script src="static/js/react-dom.production.min.js"></script>
<script src="static/js/ui.js"></script>
<script src="static/js/main.js"></script>
<script src="static/js/interact.min.js"></script>
<script src="static/js/highlight.min.js"></script>
<script src="static/js/katex.min.js"></script>
<script src="static/js/html2canvas.min.js"></script>
<script src="static/js/code-editor.js"></script>
<script src="static/js/custom.js"></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.
@CNLHC Interesting idea on letting users configure this from their config.edn. Currently :html-options is just an unused, lower level option. I'm open to exposing it to config.edn but there is more work to do:
- A config option needs to be read from
repo-config
in this fn and passed topublishing-html
. Let's call the config option:publishing/html-options
- Since a new config option is added, a new entry needs to be added to https://github.com/logseq/logseq/blob/master/src/resources/templates/config.edn. No need to set a default value but some documentation describing :before-scripts would be helpful
- Since a new config option is added, a new entry needs to be added to this var to validate the new config value
With these updates, a user can configure html-options in their config.edn and it will work with publish-spa or the built-in export. There won't be a need for logseq/publish-spa#27
@@ -120,7 +120,8 @@ necessary db filtering" | |||
} | |||
} | |||
}(window.location))"] | |||
;; TODO: should make this configurable | |||
(map #(identity (let [{:keys [src,content]} %] |
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 surprised hiccup supports this. Regardless, we shouldn't be injecting a seq in a vector. Would prefer that the scripts in :body are built dynamically e.g. (into [:body [:div {:id "root"}]] scripts)
where scripts
is the user configured ones and all the default ones below
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 surprised hiccup supports this. Regardless, we shouldn't be injecting a seq in a vector. Would prefer that the scripts in :body are built dynamically e.g.
(into [:body [:div {:id "root"}]] scripts)
wherescripts
is the user configured ones and all the default ones below
Sorry I do not get you. What do you mean by "injecting a seq in a vector"? I guess the seq is the user defined scripts (or scripts-before) ?
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 current map
is returning a seq which results in:
[:body
[:div {:id "root"}]
...
;; from scripts-before
'([:script {:src ""} ""])
[:script {:src "static/js/react.production.min.js"}]
...
All the scripts-before scripts should look like the existing :script vectors
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.
Ah I got you this time. I have added a new function to fullfill this.
Signed-off-by: Liu Hancheng <liuhancheng.cn@gmail.com>
Signed-off-by: Liu Hancheng <liuhancheng.cn@gmail.com>
e6a2435
to
f8b27f5
Compare
@logseq-cldwalker I fixed all the lint error, may be you can review this PR again when you are free |
Currently generated html has fixed scripts list, sometimes the user may want to insert extra scripts to their generated html (e.g. google analytics script). It is a good idea to make the scripts list configurable.