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.

Sign up for GitHub

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

Jump to bottom

Move data-theme from body tag to html tag #2559

Open
Hugos68 opened this issue Mar 18, 2024 · 5 comments
Open

Move data-theme from body tag to html tag #2559

Hugos68 opened this issue Mar 18, 2024 · 5 comments
Labels
feature request Request a feature or introduce and update to the project.
Milestone
v3.0 (Next)

Comments

@Hugos68
Copy link
Contributor

Hugos68 commented Mar 18, 2024

Describe the feature in detail (code, mocks, or screenshots encouraged)

Let's say I want to build a static website (no server) for my blog and I'd like the users to be able to swtich theme.
Skeleton currently has it's theme defined through a data attr on the body tag. This means we can simply swap out the data-theme attribute and see a theme switch. All goes well and it works perfectly.
Now I want to persist the theme change in localStorage.

Ofcourse because we don't want a flash of unstyled/wrongly styled content we need to load the theme before the body element loads. So we do localStorage.getItem('theme'); we get the theme and then we need to set it on the body element.

Uh oh, the body element hasn't loaded yet! We now need to wait for the body element to load with the wrong theme, and then change it to the theme.

current:

<html>
     <head>
         <script>
              const theme = localStorage.getItem('theme');
              document.body <-- undefined
         </script>
     </head>
    <body data-theme="...">...</body>
</html>

proposed:

<html data-theme="...">
     <head>
         <script>
              const theme = localStorage.getItem('theme');
              document.documentElement.setAttribute('data-theme', theme);
         </script>
     </head>
    <body>...</body>
</html>

You can see in the body version it's impossible to set the theme beforehand, however, when moving the data-attribute to html we can determine and set the theme before the body is loaded.

What type of pull request would this be?

Enhancement

Provide relevant links or additional information.

No response
@Hugos68 Hugos68 added the feature request Request a feature or introduce and update to the project. label Mar 18, 2024
@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Mar 18, 2024
@kinglouie
Copy link

kinglouie commented Apr 10, 2024 •
edited

I put the localstorage theme code first thing in the body tag. Works flawlessly without flash of wrong theme.

Example: https://github.com/sveltejs/svelte/blob/main/sites/svelte.dev/src/app.html

@Hugos68
Copy link
Contributor Author

Hugos68 commented Apr 10, 2024

Hey @kinglouie Thanks for the comment but like I said, though it happens so quickly on smaller sites, if your site is reasonably large and you will load the body first you might notice a flash of unstyled/wrongly styled content. I think we can both agree determining the style before the body tag is even loaded would be optimal.

@endigo9740
Copy link
Contributor

My concern with this isn't so much a technical one, but one of education. Users will need to be informed that we diverge from Tailwind's recommend here. All their examples show the .dark class appended to the HTML element. Their toggle code shows changing this.

https://tailwindcss.com/docs/dark-mode#supporting-system-preference-and-manual-selection

@Hugos68
Copy link
Contributor Author

Hugos68 commented Apr 10, 2024

My concern with this isn't so much a technical one, but one of education. Users will need to be informed that we diverge from Tailwind's recommend here. All their examples show the .dark class appended to the HTML element. Their toggle code shows changing this.

https://tailwindcss.com/docs/dark-mode#supporting-system-preference-and-manual-selection

Isn't is the opposite in this case? Instead of the user defining theme and mode in different places they both are applied to the HTML element. The only confusing part is that Skeleton calls dark/light "mode" and Tailwind calls this "theme". (IMO Tailwind is in the wrong here, but whatever)

@endigo9740
Copy link
Contributor

endigo9740 commented Apr 10, 2024 •
edited

The value of theme is just what they use as their localstorage key in the example. It's poor semantics on their part!

But what I'm referring to is this:

document.documentElement.classList.add('dark')

This modifies the <html> tag, not the <body>.

I bring this up as folks that are familiar with Tailwind may be used to setting/adjusting this value on the <html> element, per the official Tailwind docs recommending this approach. We can inform folks that we differ from the Tailwind norm here, but just stating the obvious that this may trip up some folks and cause some support issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Request a feature or introduce and update to the project.
Projects
None yet
Milestone
v3.0 (Next)
Development

No branches or pull requests
3 participants
@endigo9740 @kinglouie @Hugos68