Settings changes #170

techniq · 2024-01-03T12:55:33Z

Rename settings({ theme: {} }) to settings({ classes: {} })
Use settings({ currentTheme: 'themeName' }) to set the theme (see Design tokens / Dark mode #66 / Design tokens / Theme support (with dark mode) #146), specifically ThemeButton ATM
Update <Settings> component to use top-level object properties as props. So <Settings settings={{ theme: '...', classes: {...}, format: {...} }}> becomes <Settings theme="..." classes={{ ... }} format={{ ... }}>

The text was updated successfully, but these errors were encountered:

techniq · 2024-01-03T12:56:17Z

cc: @jycouet @dimfeld - Some thoughts related to work you've done, or considering doing :)

dimfeld · 2024-01-03T18:56:03Z

I think the main issue I see here is interaction between the theme set here and the default "system" (prefers-color-scheme) setting for dark mode. That could be resolved by changing theme to be something like:

type ThemeSetting = string | { light: string; dark: string };

In this case, a simple string would effectively disable the default prefers-color-scheme detection, but when both a light and dark theme are selected then it could choose appropriately. Of course, in all cases a theme setting in localStorage would override this.

What do you think?

techniq · 2024-01-03T19:30:27Z

Trying to think this through. Ultimately the source of truth is the data-theme attribute on <html> (or lack there of) and is how the theme is activated via CSS. localStorage / Svelte store is only for the setting to remain between refreshes (and to actually update the DOM when changing). It does need to initialize the value, which is maybe the challenge with SSR we're talking about.

Last wins (if data-theme is set, it will be used (3), else if prefers-color-scheme :dark is true, it will be used (2), all else, the :root (aka light) will be used. It's why the light theme is typically registered as both the default (:root) and explicitly using [data-theme=light]

svelte-ux/packages/svelte-ux/src/lib/plugins/tailwind/theme.cjs

Lines 117 to 119 in 0b67ce5

    
           // Also register first and second them by name AFTER @media for precedence 
        
           cssThemes[`[data-theme=${rootThemeName}]`] = processThemeColors(themes[rootThemeName]); 
        
           cssThemes[`[data-theme=${themeName}]`] = processThemeColors(themeColors);

The themes should be registered in the tailwind config as...

{
  ux: {
    themes: {
      light: { ... }, // will be `:root` and `[data-theme=light]`
      dark: { ... }, // will be `prefers-color-scheme: dark` and `[data-theme=dark]`
      another: { ... }, // will be `[data-theme=another]`
      more: { ... },   // will be `[data-theme=more]`
      ...
  }

I'm not sure if we're talking about exactly the same thing, but I think so :). The ThemeSettings should only ever be a single value OR undefined/null, which falls back to the system setting (:root or prefers-color-scheme: dark).

So I think there are a few things:

Persisting the theme value between refreshes (localStorage or cookie)
Reading the theme value on hydration (if needed)
Changing the theme (driven by <html data-theme="..."> and the class="dark" which is for Tailwind dark: variant (coincides with the theme's use of color-scheme: light | dark
Knowing which themes are available (grouped by light and dark schemes. Could be a single theme per scheme ([light] theme for the light scheme, and ['dark'] theme for the dark scheme. Could also have more themes per scheme ['light', 'corporate', 'winter', ...]
Probably something else

As I write this, I'm seeing the issue with <Settings> being used for setting the theme via context, as you couldn't rely on the <html data-theme>, but really it's not tied to <html> but the closest element with the data-theme attribute, so it should still just work... (I think). See Daisy UI theme docs

Does that help any, or am I way off one what you are asking :). Worst case it helps me brain dump what is in my head some.

dimfeld · 2024-01-03T19:33:53Z

What were you intending settings({ theme: 'a-theme' }) to actually do then? I was assuming it would provide a default value which would then be set in <html data-theme="..."> if you didn't want plain old "light" or "dark" but it sounds like you had something else in mind.

That is, the theming code would do the same resolution that it currently does, but instead of defaulting to "light" when it didn't have a better choice it would use "aqua" instead. That "default theme" behavior is also why I suggested the ability to provide different values for light and dark.

dimfeld · 2024-01-03T19:50:57Z

OTOH if your intent was to always have "light" and "dark" be the default light and dark themes, then I think we don't need a theme argument to settings at all.

techniq · 2024-01-03T20:01:10Z

My thought for settings({ theme: 'a-theme' }) was just to have a way to persist to localStorage what is currently in <ThemeButton>. Basically theme Button needs to just be the UI (and probably renamed to <ThemeSelect> and have a <ThemeSwitch> for light/dark toggling only.

Ultimately I think we just need to a Svelte store to do that kind of work, but then a context to be able to access it from within ThemeSelect, ThemeSwitch, etc to update <html data-theme> (or possibly a different element if we wanted to "scope" the theme). This smelled of <Settings> but maybe not.

I do intend for light and dark themes to be the default light and dark themes, and the first theme to always be the default (if you only wanted a dark mode for your site, you would just set the first one as dark). The second one is the default dark theme for the dark scheme, and then everything after that is whatever you want.

We could maybe break the tailwind config into separate by scheme:

{
  ux: {
    themes: {
      light: ['light', 'corporate', ...],
      dark: ['dark', 'night', ...],
    }
  }
}

we would still need a way to read these in JS to build the list in <ThemeSelect>, and would need to re-work the tailwind plugin's injectThemes() slightly for this new structure (but wouldn't be bad).

dimfeld · 2024-01-03T20:21:58Z

Yeah that's right. We do need a component (SsrThemeApplier or something) that can handle injecting the head script snippet since that has to be done via a svelte:head tag in order to work properly in SSR. I would probably instantiate this component inside the Settings component for ease of use, but keep it separate enough that users who prefer to use the function version of settings can do so too.

But everything else can be done with a store, and the application of the value to data-theme, localStorage, etc. can be done by subscribing to that store. The ThemeButton and so on would only interact with the "current theme" by reading and writing that store.

or possibly a different element if we wanted to "scope" the theme

I think this could be done by just passing a store into any of the Theme controls, and have it use that store instead of the global theme store. Applying the value could just be done as <div data-theme={$myCustomTheme}>. Then you could use a localStorageStore if you wanted to persist it. I'm not sure how you would make SSR work in this case though; that seems trickier.

So with all this in mind, if we aren't adding a new theme argument to settings, do we still want to rename the existing one to classes? I have a commit already that does this but easy enough to back out.

techniq · 2024-01-03T20:39:18Z

I think renaming to classes makes more sense from a readability perspective IMO. settings.classes/etc I think.

changing...

const theme = getComponentTheme('Button');

const classes = getComponentClasses('Button');

would interfere with

export let classes: {
    root?: string;
    icon?: string;
    loading?: string;
  } = {};

I'm not against pulling settings directly here... and then referencing using settings.classes('Button').nav vs theme.nav but not sure if typescript will be more challenging. It also lengthens the access everywhere.

maybe:

const settingClasses = getComponentClasses('Button');

<nav class={settingsClasses.nav} />

I know we're pulling all settings in some Date components now, so maybe doing that at the top would be best...

const settings = getSettings();
const settingClasses = settings.classes['Button'];

or something. Just spit balling ATM. I'm sure you've done something in your PR already :).

In short, yes, let's still rename settings.theme to settings.classes. Will be less confusing that they are not related to the theme, and allows us to use theme if we ever decided to.

dimfeld · 2024-01-03T20:44:20Z

const settingClasses = getComponentClasses('Button');

This is what I was starting to do. Was going to check in with you before changing every single file but that sounds good to me :)

techniq · 2024-01-03T20:54:48Z

I know that pain, and thanks :)

techniq · 2024-01-03T21:04:24Z

@dimfeld side note, I just remembered that using data-theme="..." on non-top level <html> is how the theme selector previews work :)

_	_

dimfeld · 2024-01-03T21:05:15Z

Yep, but those don't ever change so there's no issue there.

techniq · 2024-01-03T21:06:37Z

In practice, I think only the top-level <html data-theme=""> would ever change, and likely other usage would be hard coded to say <html data-theme="light"> or whatever, and like you mentioned, if you want it dynamic, just leave it to the user to control that however they want (put in local storage with whatever key, etc)

This was referenced Jan 3, 2024

Change "theme" to "classes" in Settings #172

Merged

Update Settings component to expose each settings value as its own component prop #177

Merged

techniq closed this as completed Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings changes #170

Settings changes #170

techniq commented Jan 3, 2024 •

edited

Loading

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024 •

edited

Loading

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024 •

edited

Loading

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

Settings changes #170

Settings changes #170

Comments

techniq commented Jan 3, 2024 • edited Loading

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024 • edited Loading

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024 • edited Loading

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

techniq commented Jan 3, 2024

dimfeld commented Jan 3, 2024

techniq commented Jan 3, 2024

techniq commented Jan 3, 2024 •

edited

Loading

dimfeld commented Jan 3, 2024 •

edited

Loading

techniq commented Jan 3, 2024 •

edited

Loading