v4-beta.1: darkModeSelector class only fully works on <html> Element

m-meier commented 6 months ago

Describe the bug

According to the documentation example it should be possible to toggle the class for the darkModeSelector on the body element. However this leads to mixed results. Some components change partially to dark mode, others don't at all. It seems to only work on the element for all cases. It would be great if it would work on body elements since many plattforms use the body tag for dark mode classes.

In the Stackblitz reproduction I have added a button to toggle the class on the body element (according to the docs). Here's what I could observe:

The checkbox is not affected at all
The listbox changes font color but not background color
The selectbox fully changes the dropdown to dark mode, but the input itself stays in light mode.

Reproducer

https://stackblitz.com/edit/vitejs-vite-pnhoaf?file=src%2FApp.vue

PrimeVue version

4.0.0-beta.1

Vue version

3.x

Language

ALL

Build / Runtime

Vite

Browser(s)

Chrome 123

Steps to reproduce the behavior

Toggle darkModeSelector class on body.

Expected behavior

Preset switches to dark mode when toggling the class on the body element.

m-meier commented 6 months ago

Okay, i continued to look into this issue. From what I gathered PrimeVue does not actually look at the darkModeSelector defined anywhere but instead creates additional CSS with the darkModeSelector as prefix. In this CSS the relevant semantic tokens like surface-100 etc are overwritten with the dark mode values. Since the specificity is greater with the prefix, these values will be used if the class is present.

Now comes the problem: The component tokens are using the semantic tokens. However they are defined at :root, so basically the html tag or documentElement. If the darkmodeSelector is deeper in the DOM (e.g. on the body tag), the overwritten variables will also be placed at the deeper element (body). Now I suspect that since the component tokens are at the root they will also only use variables specified at the root level. And those are always the light mode variables if the darkModeSelector is at a deeper level.

If I understand everything correct that means that for a darkModeSelector to work on the body level there are two possible solutions:

All tokens have to be moved down to the body element. This is probably not a desirable solution
The prefix is adjusted. In my test it works if you expand the prefix to not only contain the darkModeSelector, but also a :root:has([darkModeSelector]) selector.

The second solution seems to require very little effort and sounds like a viable solution. A concrete example: Assume: darkModeSelector: '.dark-theme'

Currently this generates this css: .dark-theme { --p-surface-0: #ffffff, .... } With the change this would become: .dark-theme, :root:has(.dark-theme) { --p-surface-0: #ffffff; .... }

ixxie commented 4 months ago

If functionally, there is not advantage to using the body selector here, why no just update the docs to explain you must use the html element, and likewise update the toggle example?

m-meier commented 4 months ago

While that certainly would be an easy solution I think supporting a more flexible selector has a few advantages:

One of the main characteristics of PrimeVue is that it only styles its components, not the whole page. This makes it far easier to integrate into existing applications (event third party applications) where you can't simply switch the class to the root element. This is actually what brought me to PrimeVue.
Having a class selector only on the root means it is outside of your application and you can't use class data binding and are always forced to use standard javascript DOM manipulation in your Vue application.
This is more of a personal thing I guess, but I prefer visual classes on the body or application frame and use the document root only for variable definitions.

But if the team decides to only support classes on the root it will at least still be possible to define ':root:has(.dark)' as the selector, that seems to work fine. Not very intuitive, but maybe it could be mentioned in the docs.

Michael2109 commented 3 months ago

I've been having an issue where the documentation says to use

document.body.classList.toggle('app-dark')

but it doesn't work as it needs to be applied to the html element.

Could the documentation be updated to have a working example? It will stop other people having the same issue.

xarthurx commented 3 months ago

Is there any update on this issue? I saw this is removed from v4.0.0.

And if I search the code base, it seems the darkModeSelector only appears in the showCase folder, but not any implementation. https://github.com/search?q=repo%3Aprimefaces%2Fprimevue+darkModeSelector&type=code

In my local settings, linting shows that darkModeSelector is not in PrimeVueOptions:

mertsincan commented 3 months ago

Hi @xarthurx, the warning is correct. Please move it into theme option; https://primevue.org/theming/styled/#theme. Also, our all styled implementation is in https://www.npmjs.com/package/@primeuix/styled

xarthurx commented 3 months ago

Hi @xarthurx, the warning is correct. Please move it into theme option; https://primevue.org/theming/styled/#theme. Also, our all styled implementation is in https://www.npmjs.com/package/@primeuix/styled

Oh, this saves my whole day! For nuxt.config.ts, it is a bit confusing and I finally realized the issue:

cardin commented 1 month ago

After reading https://github.com/primefaces/primevue/blob/master/apps/showcase/app.vue, https://github.com/primefaces/primevue/blob/master/apps/showcase/themes/app-theme.js and https://primevue.org/theming/styled/#Palette, the following worked for me:

nuxt.config.ts

import Aura from "@primevue/themes/aura";
primevue: {
  options: {
    theme: {
      preset: Aura,
      options: {
        darkModeSelector: '.darkmode'
      }
    }
  }
}

app.vue

import {
  palette,
  updatePrimaryPalette,
  updateSurfacePalette,
} from "@primevue/themes";

document.querySelector("html")!.classList.toggle("darkmode");
updatePrimaryPalette(palette("{blue}"));
updateSurfacePalette({ dark: palette("{stone}") });

primefaces / primevue