Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make HTML template configurable: (Make <head> tag fully customizable) #13

Closed
jbmoelker opened this issue Aug 23, 2020 · 12 comments 路 Fixed by #35
Closed

Make HTML template configurable: (Make <head> tag fully customizable) #13

jbmoelker opened this issue Aug 23, 2020 · 12 comments 路 Fixed by #35
Labels
enhancement New feature or request
Projects

Comments

@jbmoelker
Copy link

jbmoelker commented Aug 23, 2020

馃憦 for starting a static site generator using a modern framework with 0kb client-side JS by default and optional client-side hydration!

Currently the HTML template is statically defined in Page.ts:

    page.htmlString = `<!DOCTYPE html>
      <html lang="en">
        <head>
          <meta charset="UTF-8" />
          <meta name="viewport" content="width=device-width, initial-scale=1" />
          ${page.headString}
        </head>
        <body class="${page.request.route}">
          ${layoutHtml}
          ${page.hydrateStack.length > 0 ? page.beforeHydrate : '' /* page.hydrateStack.length is correct here */}
          ${page.hydrateStack.length > 0 ? page.hydrate : ''}
          ${page.customJsStack.length > 0 ? page.customJs : ''}
          ${page.footerStack.length > 0 ? page.footer : ''}
        </body>
      </html>
    `;

It would be great if this template would be configurable, so for instance html[lang] could easily be changed.
Maybe this can be offered through a hook, or by introducing a src/template.html file like in Sapper.

This would help with #6, as I'm currently forced to overwrite the html[lang] with a hook:

  {
    hook: 'html',
    name: 'setHtmlLang',
    description: 'Overwrite html[lang="en"] with html[lang="{ request.locale }"]',
    priority: 50,
    run: ({ htmlString, request }) => {
      return {
        htmlString: htmlString.replace('<html lang="en">', `<html lang="${ request.locale }">`),
      };
    },
  },
@halafi halafi added the enhancement New feature or request label Aug 23, 2020
@nickreese
Copy link
Contributor

@jbmoelker With Sapper how would you end up setting the request.locale? Would you need a different template for each language?

Possible Elder.js Solutions:

I'll continue thinking on this issue, but the three solutions I can see are:

  1. Using a hook like you have done.
  2. Offering a function that could be defined in the elder.config.js which would allow users to define a function which overwrites what is done in Page.ts. (even as I write this I don't like it)
  3. We could add a hook called compileHtml or mergeHtml and move the logic out of Page.ts and into a Elder.js hook. This would allow users to disable the system defined default in elder.config.js and then register their own function on that hook.

Issues With Each Option:

  1. This solution works, but puts the work on the developer. We could document this for others looking to do localization.
  2. Bad option. The complexity it would add outweighs the benefits.
  3. This adds the most flexibility and moves Elder.js core logic to a hook which is a good move... but the trade offs are that it adds complexity to the already confusing hook interface (how does compileHtml differ from html?) and we'd also need a limitation to only allow a single function registered on this hook to prevent unexpected chaos. Also adds a documentation burden.

Questions to investigate:

  • What other types of things would need to be customized like the html[lang="en"] tag? Possibly itemscope="" itemtype="http://schema.org/WebPage" if people were using old schema.org. Any others?
  • What are the limitations/downsides of using the currently existing html hook as shown above besides execution priority?

Notes on Hooks:

When building Elder.js we started out with 27 hooks. There was a hook for everything. In sharing it with a private group of devs the resounding feedback was that hooks are powerful but there were just too many. The current implementation is the smallest amount of hooks we could identify that still allowed us to solve all of our edge cases on elderguide.com.

As of this writing, I think we should be very cautious about adding additional hooks and instead explore giving users extremely fine grained control of the priority functions are executed on each hook even allowing them to override the priority defined by plugin hooks.

For example, the biggest downside to using the setHtmlLang function defined above is that more small functions a project has the more the user needs to have fine grained control over the execution order.

As it currently stands the 1-100 priority scale (with 1 being highest priority) is enforced by Elder.js and it sets a cap on the priority functions are run at. Long term we may want to revisit this scale and see if the strict enforcement of it makes sense for user defined functions. (We could also invert the scale and allow users to define a priority with 101 or higher.)

@nickreese
Copy link
Contributor

@jbmoelker So the more I've thought about this the more I believe your current hook implementation is actually the desired one until there is a more fully fleshed out plan for i18n and community plugins supporting it.

That said, another possible low hanging fruit solution would be to have Elder.js look for a request.locale or request.lang and set the value there on the html. If it doesn't exist it defaults to <html lang="en">.

I think living this issue open for more conversation about customizing the HTML template is worthwhile. If anyone reading this has ideas, we'd love to hear them.

@nickreese
Copy link
Contributor

@jbmoelker I'm going to close this as I think a hook is the correct way to solve this specific issue.

@jbmoelker
Copy link
Author

Hi @nickreese, you raise some good questions :) I'm not sure how you could pass more data to a Sapper template (I wasn't too satisfied with Sapper and haven't used it much since). I'm not sure I'm a big fan of my own solution using string replace in a hook. I imagine a hook could still work. It would maybe be nice if page.htmlAttributes would be available inside hooks, so you can properly manipulate them rather than having to use string replace / regex matching.

@nickreese
Copy link
Contributor

nickreese commented Sep 3, 2020

@jbmoelker that wasn鈥檛 a solution I considered. What hook would you suggest it on? request?

@akvadrako
Copy link

I think living this issue open for more conversation about customizing the HTML template is worthwhile. If anyone reading this has ideas, we'd love to hear them.

Does that mean it would be appropriate to open a new issue to continue this discussion?

I would like to change the "viewport" meta tag. I think it would be great if you would just remove the meta tags from the template:

        <head>
          ${page.headString}
        </head>

They can anyway be easily customized in the <svelte:head> tags.

@nickreese
Copy link
Contributor

@akvadrako Totally doable. We'll add <meta charset="UTF-8" /><meta name="viewport" content="width=device-width, initial-scale=1" /> as defaults that can be disabled by disabling the hook. The majority of the discussion was mainly about the <html tag.

@nickreese nickreese reopened this Sep 9, 2020
@nickreese nickreese changed the title Make HTML template configurable Make HTML template configurable: (Make <head> tag fully customizable) Sep 9, 2020
@nickreese
Copy link
Contributor

@akvadrako Initial commit here, but for the sake of future proofing and remembering somewhere that the char encoding should always be first, I'm going to break these into two different hooks before releasing.

@nickreese
Copy link
Contributor

@akvadrako pushed with v0.1.5. Also fixed a bug where stacks were being processed backwards.

Doesn't fix the HTML lang issue, still open to coming up with a more official plan.

@nickreese
Copy link
Contributor

For anyone that finds this, if you want to remove both meta tags, add elderAddMetaCharsetToHead and elderAddMetaViewportToHead to your hooks.disable array in your elder.config.js.

@akvadrako
Copy link

@nickreese thanks for the quick fix. It's working well for me.

@nickreese nickreese linked a pull request Sep 22, 2020 that will close this issue
34 tasks
@nickreese
Copy link
Contributor

This will be in v1. Elder.js now uses the compileHtml hook which the user can disable and add their own template.

@nickreese nickreese added this to Done in Elderjs Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Elderjs
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants