[Starlight] Auto-set theme from param #26412
Conversation
|
This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:
|
|
Preview URL: https://c72e6ce3.preview.developers.cloudflare.com |
| const params = new URL(location.href).searchParams; | ||
| const theme = params.get("preferred-color-scheme"); | ||
| if (theme === "dark" || theme === "light") { | ||
| document.documentElement.dataset.theme = theme; |
There was a problem hiding this comment.
This line doesn't appear to do anything based on testing. Keeping it for the moment, but probably superceded by the local storage setting line in 10.
Ideally, I'd prefer to keep this behavior (9 over 10) -- and just load this time in this theme -- instead of setting the local variable to avoid someone getting docs theming set unexpectedly
| } | ||
| </script> | ||
|
|
||
| <Default><slot /></Default> |
There was a problem hiding this comment.
How does this play with Starlight's version? Which one gets loaded first?
You might want to not use the Default and combine the existing logic to give you something along the lines of
const urlTheme = new URL(location.href).searchParams.get("preferred-color-scheme");
const storedTheme =
typeof localStorage !== 'undefined' && localStorage.getItem('starlight-theme');
const theme =
urlTheme || storedTheme ||
(window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark');
// do the same logic as Starlight's from here on outI'm guessing the standard way to override Starlight comps won't allow that? And thus we need to replace it instead of adding to it?
There was a problem hiding this comment.
Also, I think the following use case could happen:
- The URL is
theme=light, which loads the light theme. - User clicks the doc's theme picker and chooses the dark theme.
- Now the URL has light but the doc is dark.
Not sure we care about that though. It in theory could break copy/pasting of links depending on how Astro handle relative links when there's a query param. (I assume it ignores it.) If this case matters, then something in ThemeSelect likely needs to be overridden as well.
|
Updating the query param works great on initial load, when the docs are loaded in an iframe. However, if you start navigating the docs in the iframe, and then switch themes, the iframe's src is reloaded and you lose your navigation.
With the url concerns here as well #26412 (comment), is there any appetite for reading from either reading from a shared cookie and/or include a postMessage listener. I don't love the postMessage, only because it would be functionality that's only used when the docs are loaded in an iframe. I wouldn't mind trying to clean up the original PR a bit, so the changes are a bit tidier, and not as complex. I will admit i hit "enter" a bit prematurely on that PR, as i was trying to open it in Draft mode, originally |
3 participants
Summary
Diff implementation of #26409