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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to change the html tag? #82

Closed
geovannimp opened this issue Feb 3, 2024 · 19 comments
Closed

How to change the html tag? #82

geovannimp opened this issue Feb 3, 2024 · 19 comments
Assignees
Labels

Comments

@geovannimp
Copy link

I'm using tailwind and my page always blink switching from light theme to dark theme on client render. I would like to add the dark class by default to the html tag, but I can not find how to change it on server side.

@brillout
Copy link
Member

brillout commented Feb 3, 2024

How about this?

// /pages/+Html.jsx

export function Html({ head, body }) {
  return <>
    <!DOCTYPE html>
    <html className={whateverYouWant}>
      <head>
        {head}
      </head>
      <body>
        {body}
      </body>
    </html>
  </>
}

Let me know if that works for you, I'll then implement and pre-release it. I'll stable release it after the vike-vue team agrees on the design.

Edit: we decided against this in order to enable Vike extensions to add <html> attributes.

@brillout brillout added enhancement ✨ New feature or request highest-prio 🔥 labels Feb 3, 2024
@brillout brillout self-assigned this Feb 3, 2024
@geovannimp
Copy link
Author

I like it, but the Html could be a config like the Head.
Actually with a Html config, you don't even need the Head config 🤔

@geovannimp
Copy link
Author

Thinking longer, I think the Head still useful. But I'm still in favor of using the Html as a config property.

@brillout
Copy link
Member

brillout commented Feb 5, 2024

It is a config just like Head is.

I think there is room for improvement but I can't think of a better solution. The new setting Html would at least unblock you.

I'm inclined to first focus on unblocking users and then, later, polishing the DX.

Ideas welcome.

@geovannimp Let me know if you want to be unblocked sooner rather than later, I'm happy to prioritize a pre-release.

@AurelienLourot WDYT?

@lourot
Copy link
Contributor

lourot commented Feb 5, 2024

This sounds good to me. Let's try to avoid hard breaking changes along the way if they are not really necessary 🙏

@geovannimp
Copy link
Author

@brillout I won't be releasing the product I'm working on any time soon, so you can prioritize as you want. But thank you for offering.

@brillout
Copy link
Member

brillout commented Feb 6, 2024

👍 I'll implement this once I fixed Dani's blockers.

Let's try to avoid hard breaking changes along the way if they are not really necessary 🙏

Ok 👍

@fortezhuo
Copy link

fortezhuo commented Aug 8, 2024

I think there is room of improvement htmlAttributes by allowing pageContext consumed. We can get theme class from pageContext that value of theme fetched from req.cookies

function getPreference(cookies: Record<string, any>): Preference {
  return cookies?.preference ? JSON.parse(cookies.preference) : {theme:"light"}
}

export const getPageContext = function (req: FastifyRequest) {
  const preference = getPreference(req.cookies)

  return {
    urlOriginal: req.originalUrl || req.url,
    user: req.user,
    preference,
  }
}

// server middleware using vike-node
import vike from "vike-node/fastify"
...
  await instance.register(
    vike({
      pageContext: option.getPageContext,
      static: {
        root: option.distClient.root,
      },
    }),
  )
...

// from vike-react
function getTagAttributes(pageContext: PageContextServer) {
  let lang = getHeadSetting('lang', pageContext)
  // Don't set `lang` to its default value if it's `null` (so that users can set it to `null` in order to remove the default value)
  if (lang === undefined) lang = 'en'

  const bodyAttributes = mergeTagAttributesList(pageContext.config.bodyAttributes)
  // const htmlAttributes = mergeTagAttributesList(pageContext.config.htmlAttributes)
  const htmlAttributes = getHeadSetting('htmlAttributes',pageContext)

  const bodyAttributesString = getTagAttributesString(bodyAttributes)
  const htmlAttributesString = getTagAttributesString({ ...htmlAttributes, lang: lang ?? htmlAttributes.lang })

  return { htmlAttributesString, bodyAttributesString }
}

@geovannimp
Copy link
Author

geovannimp commented Aug 8, 2024

Just in case someone have the same problem with tailwind, I fixed my problem adding this script to my Head component.

      <script
        dangerouslySetInnerHTML={{
          __html: `
              (function() {
                const systemTheme = window.matchMedia('(prefers-color-scheme: dark)')
                  .matches
                    ? 'dark'
                    : 'light';
                document.documentElement.classList.add(systemTheme);
              })();
          `,
        }}
      />

@brillout
Copy link
Member

https://vike.dev/htmlAttributes

Let us know if anything doesn't work for your use case. We look forward to gathering feedback on the new improvements to head tag management.

I think there is room of improvement htmlAttributes by allowing pageContext consumed.

Done.

@fortezhuo
Copy link

fortezhuo commented Aug 13, 2024

Hi @brillout

import type { Config } from "vike/types"
import vikeReact from "vike-react/config"

const Head = "import:../components/root/head.tsx:Head"

export default {
  passToClient: ["user", "preference"],
  htmlAttributes: (pageContext) {
    return { class: "dark" }
  },
  Head,
  extends: [vikeReact],
} satisfies Config

Got error message like this :

9:06:40 AM [vike][request(1)][Wrong Usage] htmlAttributes defined by /src/pages/+config.ts must be defined over a so-called "pointer import", see https://vike.dev/config#pointer-imports

And htmlAttributes also not working using "pointer import"

@brillout
Copy link
Member

@fortezhuo Minimal reproduction welcome.

@pandringa
Copy link

pandringa commented Aug 13, 2024

Hi @brillout - thanks for your work on this handy feature!

I'm just discovering this thread and encountering the same issue - whenever I try to set htmlAttributes as a function of pageContext (rather than an object literal), vite build throws the following:

[vike:importUserCode] Could not load virtual:vike:pageConfigValuesAll:server:/pages/index: 
Error: [vike][Wrong Usage] htmlAttributes defined by /pages/+config.js must be defined over a so-called "pointer import", see https://vike.dev/config#pointer-imports
    at logJsonSerializeError (file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:195:5)
    at valueToJson (file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:171:9)
    at serializeWithJson (file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:141:27)
    at serializeConfigValueSource (file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:74:21)
    at file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:55:54
    at Array.forEach (<anonymous>)
    at file:///.../node_modules/vike/dist/esm/shared/page-configs/serialize/serializeConfigValues.js:52:18
    at Array.forEach (<anonymous>) {
  code: 'PLUGIN_ERROR',
  plugin: 'vike:importUserCode',
  hook: 'load'
}

I've also tried setting it via a separate +htmlAttributes.js file, or via the useConfig() hook... in those cases the build finishes but it doesn't seem to set the attributes?

If relevant, I'm on [email protected], [email protected], and [email protected].

My +config.js file looks like:

import vikeReact from 'vike-react/config';

export default {
  extends: [vikeReact],
  prerender: true,
  ssr: true,
  passToClient: ['context'],
  htmlAttributes: ({ data }) => ({
    class: data.pageClasses,
  }),
};

@fortezhuo
Copy link

@brillout

Here the minimal reproduction using batijs
https://github.com/fortezhuo/vike-demo/blob/main/pages/%2Bconfig.ts

Server listening on http://localhost:3000
7:31:14 PM [vike][request(1)] HTTP request: http://localhost:3000/
7:31:14 PM [vike][request(1)][Wrong Usage] htmlAttributes defined by /pages/+config.ts must be defined over a so-called "pointer import", see https://vike.dev/config#pointer-imports
7:31:14 PM [vike][request(1)] HTTP response http://localhost:3000/ ERR
7:31:17 PM [vike][request(2)] HTTP request: http://localhost:3000/
7:31:17 PM [vike][request(2)][Wrong Usage] htmlAttributes defined by /pages/+config.ts must be defined over a so-called "pointer import", see https://vike.dev/config#pointer-imports
7:31:17 PM [vike][request(2)] HTTP response http://localhost:3000/ ERR
7:31:18 PM [vike][request(3)] HTTP request: http://localhost:3000/sw.js
7:31:18 PM [vike][request(3)][Wrong Usage] htmlAttributes defined by /pages/+config.ts must be defined over a so-called "pointer import", see https://vike.dev/config#pointer-imports
7:31:18 PM [vike][request(3)] HTTP response http://localhost:3000/sw.js ERR

@brillout
Copy link
Member

That’s expected: follow the link. Let me know if you find in unclear.

@fortezhuo
Copy link

fortezhuo commented Aug 13, 2024

I just created file named htmlAttributes.ts

export default (pageContext: any) => {
  return { class: "dark" };
};

And here the +config.ts

import vikeReact from "vike-react/config";
import type { Config } from "vike/types";
import Head from "../layouts/HeadDefault.js";
import Layout from "../layouts/LayoutDefault.js";

const htmlAttributes = "import:./htmlAttributes.ts:default";

// Default config (can be overridden by pages)
export default {
  Layout,
  Head,
  htmlAttributes,
  // <title>
  title: "My Vike App",
  extends: vikeReact,
} satisfies Config;

And typescript not happy with these scripts, also in browser, html does not have "dark" class name

Type 'string' is not assignable to type 'TagAttributes | ((pageContext: PageContextServer) => TagAttributes | undefined) | undefined'.ts(2322)
Config.d.ts(110, 13): The expected type comes from property 'htmlAttributes' which is declared here on type 'Config'

PR on the vike-demo repo welcome for make all setup more clear

@pandringa
Copy link

pandringa commented Aug 13, 2024

Ah, I think I understand. If the return value of pages/+config.js must be JSON-serializable, I guess an inline function definition isn't supported there? That might be helpful to clarify in the docs — something like "if you wish to define htmlAttributes as a function of pageContext, you must do it in a separate +htmlAttributes.js config file".

I still think there might be a bug in the implementation - when I define a static object in +htmlAttributes.js it seems to automatically pick up the value, but when I define a function it seems to fail.

I'm not nearly as familiar with this codebase, so I could be wrong, but I suspect the bug has something to do with this isCallable check here. Now that htmlAttributes is an inheritable, mergable config value, val is always an array like [ TagAttributes | (pageContext) => TagAttributes ] — so isCallable always returns false, even if the config value is in fact a function.

I've verified by tweaking this function locally, and it seems to work:

export { getHeadSetting };
import { isCallable } from '../utils/isCallable.js';
function getHeadSetting(headSetting, pageContext) {
    // Set by useConfig()
    {
        const val = pageContext._configFromHook?.[headSetting];
        if (val !== undefined)
            return val;
    }
    // Set by +configName.js
    const val = pageContext.config[headSetting];

    // Handle arrays of pre-merged configs
    if (Array.isArray(val)) {
        return val.map(v => 
            isCallable(v) ? v(pageContext) : v
        )
    } else {
        return isCallable(val) ? val(pageContext) : val;
    }
}

@brillout
Copy link
Member

brillout commented Aug 14, 2024

Good catch, fix pre-released as 0.5.2-commit-6168003 0.5.2-commit-a53b7c1.

Ah, I think I understand. If the return value of pages/+config.js must be JSON-serializable, I guess an inline function definition isn't supported there? That might be helpful to clarify in the docs — something like "if you wish to define htmlAttributes as a function of pageContext, you must do it in a separate +htmlAttributes.js config file".

👍 Done. And I'll do this when I'm done with the current priorities.

typescript not happy

I didn't decide yet whether I want to add import:{string} to the types.

Let me know if there is anything else.

@brillout
Copy link
Member

pre-released as 0.5.2-commit-6168003.

Use 0.5.2-commit-a53b7c1 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants