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

Set default color theme in template #1473

Merged
merged 3 commits into from
Feb 8, 2024
Merged

Set default color theme in template #1473

merged 3 commits into from
Feb 8, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Feb 8, 2024

Description

  • Fixes Both dark and light Starlight appears when JS disabled #1470

  • We are not currently setting data-theme by default, only when a small inline script runs:

    document.documentElement.dataset.theme = theme === 'light' ? 'light' : 'dark';

  • For most styles this isn’t an issue because we set dark theme variables on :root, light theme variables on [data-theme="light"]. But our light:sl-hidden and dark:sl-hidden classes expect data-theme to be set.

  • To resolve this, this PR sets data-theme="dark" in the prerendered HTML, so we are not dependent on JS running in order for those styles to work.

  • There should be no behaviour change for users running JS, only this bug fix for those without.

Alternative solutions

I considered an alternative solution of updating the dark:sl-hidden style to work when data-theme is not set:

- [data-theme='dark'] .dark\:sl-hidden {
+ :root:not([data-theme='light']) .dark\:sl-hidden {

But given that it seems like a whole class of potential bugs could be caused by missing data-theme (including potentially in user code), it seemed safest to set the default value at source.

Copy link

changeset-bot bot commented Feb 8, 2024

🦋 Changeset detected

Latest commit: 3fea4c6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Feb 8, 2024 3:43pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Feb 8, 2024 3:43pm

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Feb 8, 2024

size-limit report 📦

Path Size
/index.html 5.23 KB (+0.14% 🔺)
/_astro/*.js 21.54 KB (+0.02% 🔺)
/_astro/*.css 13.13 KB (0%)

Copy link
Member

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach is sound and this also fixes syntax highlighting for users having a preference set to light mode and JavaScript disabled 👍

@delucis delucis merged commit 29da505 into main Feb 8, 2024
9 checks passed
@delucis delucis deleted the dx-993/no-js-fix branch February 8, 2024 15:55
@astrobot-houston astrobot-houston mentioned this pull request Feb 8, 2024
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Feb 10, 2024
* main: (96 commits)
  [ci] format
  i18n(ko-KR): update `manual-setup.mdx` (withastro#1482)
  i18n(ko-KR): update `community-content.mdx` (withastro#1483)
  [ci] format
  [ci] release (withastro#1481)
  [ci] format
  Make Starlight compatible with server output mode (withastro#1454)
  [i18nIgnore] community content: article description copy edit (withastro#1408)
  [ci] format
  i18n(it): Updated plugins.md and community-content.mdx (withastro#1480)
  i18n(fr): update `resources/community-content` (withastro#1479)
  [ci] format
  i18n(it): Modified everything in the /guides folder (withastro#1456)
  i18n(it): Modified frontmatter.md and overrides.md (withastro#1457)
  i18n(es): fix syntax highlighting with `diff`-like syntax example (withastro#1477)
  Add CodingCat.dev stream video to community content page (withastro#1475)
  i18n(fr): fix frontmatter `label` default value translation (withastro#1476)
  [ci] format
  [ci] release (withastro#1474)
  Set default color theme in template (withastro#1473)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Both dark and light Starlight appears when JS disabled
3 participants