Skip to content

Conversation

@Ze0nC
Copy link
Contributor

@Ze0nC Ze0nC commented Jan 19, 2020

Hi @JohnSundell, sorry for requesting pulling so frequently.
Here is one about an API change. The idea is in early stage and I want to discuss about it with you if you are interested.

About the change

  1. To add the additionalNodes argument in head function to specify addition nodes for pages.
  2. To add a whole-site additional Nodes for head.

I really appreciate the idea of "javascript-free", but sometimes we need to embed javascript in html head, such as when using Google Analytics.
Therefore, I needed a way to add customized additional whole site nodes.

I have tried to write an extension in extension Node where Context == HTML.HeadContext in my own package. But I think it might be better to include the ability to add additional nodes in the official Publish repo.

usage example
One example is adding Google Analytics to Publish, which requires script node in head.
I have made demo plugin which uses the additionalWholeSiteHeadNodes property.

Another example is writing articles which need specific .js files. I have tried to show LaTeX equations with MathJax, so I need to add MathJaX script on some pages (not all). In this case the 'additionalNodes' argument is helpful.

Please consider the request when convienent.
Cheers,
Zhijin

@artur-ios-dev
Copy link
Contributor

Oh, I was about to create same PR 👍 Right now we end up doing a pretty much the copy of the already prepared .head() which kinda sucks.

@Ze0nC
Copy link
Contributor Author

Ze0nC commented Jan 21, 2020

Oh, I was about to create same PR 👍 Right now we end up doing a pretty much the copy of the already prepared .head() which kinda sucks.

It seems we all have this issue.

Since I want to keep using current code of my theme, I am using forked version of Publish and I have to keep it updated with upstream. It sometimes causes conflicts with other plugins.

Btw do you have any suggestion on improvement this implementation? I am wondering if there is a better way to do it...

@artur-ios-dev
Copy link
Contributor

@Ze0nC I just made my own header function for now until this PR is merged.

@Ze0nC
Copy link
Contributor Author

Ze0nC commented Jan 21, 2020

Oh, I was about to create same PR 👍 Right now we end up doing a pretty much the copy of the already prepared .head() which kinda sucks.

Off topic question: do you find any way to modify the content after generation of HTML, just like the Modifier in Plot.

@artur-ios-dev
Copy link
Contributor

artur-ios-dev commented Jan 21, 2020

Off topic question: do you find any way to modify the content after generation of HTML, just like the Modifier in Plot.

I put a STRING_KEY in my article which I replace with the actual text while building page.

Copy link
Owner

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this @Ze0nC. Just remove the static var, and I'll gladly merge this in. I agree that Plot probably needs a way to modify nodes after they were created. I'll see if I can find some time to work on figuring something out for that soon, and if you have any ideas, feel free to submit a PR. I'm really grateful for all of your contributions 🙂

/// - parameter rssFeedPath: The path to any RSS feed to associate with the
/// resulting HTML page. Default: `feed.rss`.
/// - parameter rssFeedTitle: An optional title for the page's RSS feed.
/// - parameter additionalNodes: Optional additional nodes for the page.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the page -> to add to the resulting <head> element.


public extension Node where Context == HTML.DocumentContext {
/// Additional nodes of head. These nodes will be added to head of whole site.
static var additionalWholeSiteHeadNodes: [Node<HTML.HeadContext>] = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think adding globally mutable state is a good idea. I understand the use case for this, but I think it should be solved in other ways, for example by creating a site-specific head extension which then composes these universal nodes into Publish's head(for...) call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. Global variable is not a clean way to do.

Let me consider a better implementation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just remove it for now, so that we can merge the other change in 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed it.

@JohnSundell JohnSundell added the awaiting update Awaiting an update from the PR author label Jan 29, 2020
@Ze0nC
Copy link
Contributor Author

Ze0nC commented Feb 21, 2020

Hi @JohnSundell ! I have finished my defense so now I have more free time on it!

After some considerations and trials I found it is hard for me to implement it without using global variable, so I would like to discuss with you.

First of all, we need to store the nodes in some place, and I have considered:

  • Node<HTML.DocumentContext>: This is bad since it is global. But it is the most convenient place to put global head nodes?
  • Website instance: Probably a good place, but I think 'Node' probably shouldn't appear here
  • PublishContext: Same with above.
  • HTMLFactory: Seems to be a good place. This is where HTML are generated.
  • Theme: Seems to be a good place?

I have considered for some days but haven't figured it out.
Do you have some ideas/suggestions?

Cheers,
Zhijin

@Ze0nC
Copy link
Contributor Author

Ze0nC commented Mar 2, 2020

Hi @JohnSundell. I hope you are doing well recently.
I committed a draft of new way to manage global head nodes of websites.
Here are the changes:

  1. 'ConditionalHeadNode' has been added. It defines a var node: Node<HTML.HeadContext> with a var condition: (Location) -> Bool, and var target: String which is description of Website.
  2. 'ConditionalHeadNode' has a static variable var all: [ConditionalHeadNode], which holds all head nodes to add.
  3. 2 new functions were added to extension of Website to add and get the head nodes.

When the above change, it is possible to:

  1. Add global head nodes to certain websites.

  2. Add head nodes to certain pages of website.

However, it is not perfect:

  1. Should Website protocol has functions which deal with Node?

  2. Current way to identify target websites is by using description of websites. This is not safe and Swiftly. However due to limitation of my skills I didn't find a better to do it.

  3. Instead of var all: [ConditionalHeadNode], a dictionary might be better to store the nodes.

I hope this implementation is better then previous one.

Cheers,
Zhijin

@yogeshmanghnani
Copy link

why isn't this PR merged ?

@Malauch
Copy link

Malauch commented Jan 3, 2021

I hope this PR will be merged soon.

@jjaychen1e
Copy link

I'm looking forward to this great feature! When can this PR be merged?

@t2ac32
Copy link

t2ac32 commented Dec 6, 2021

So there's no implemented way of adding a Script tag at this moment?

@dannys42
Copy link

I was looking for a solution to this as well, but I'm thinking it would be nice to have it as a plugin or additional publishing step. My reason for this is that I'd like to easily switch it on/off depending on whether I'm deploying for production or test.

@dannys42
Copy link

@Ze0nC My approach to this problem is in PR-120. It's working with raw HTML, so not type-safe in any way, but allows someone to easily make a GoogleAnalytics plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting update Awaiting an update from the PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants