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

Refactor <ThemeSelect> script to avoid double media query event listener #1734

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Apr 10, 2024

Description

  • Follow up to fix: respond to media query change events when theme is auto #1731
  • That PR introduces a media query event listener so that the colour theme can stay in sync with the system theme when using the default auto theme. But because we often have two <ThemeSelect> components on each page (nav bar and mobile menu), this meant a double listener, each updating both select menus.
  • To solve this I pulled most of the logic out of the StarlightThemeSelect class, which wasn’t actually instance-specific, so the media query event listener can just run once at the top level.
  • The diff is a bit overwhelming but it’s mainly moving the private class methods up to top-level constants. The media query gets moved from the custom element constructor to the top level so we only have one.

Copy link

changeset-bot bot commented Apr 10, 2024

🦋 Changeset detected

Latest commit: 8e37346

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 Apr 10, 2024

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

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Apr 10, 2024 7:26pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Apr 10, 2024 7:26pm

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Apr 10, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Apr 10, 2024

size-limit report 📦

Path Size
/index.html 6.1 KB (-0.04% 🔽)
/_astro/*.js 21.57 KB (-0.05% 🔽)
/_astro/*.css 13.45 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.

Thanks, this looks like a thorough refactor to me and my local tests did not spot any issues. Looks perfect to me 👍

Unrelated to this change, we should probably do a follow-up to the follow-up to fix the Property 'StarlightThemeProvider' does not exist on type 'Window & typeof globalThis'.ts(2339) TS error, I would guess it's just missing an export for declare in packages/starlight/global.d.ts.

@delucis
Copy link
Member Author

delucis commented Apr 10, 2024

Unrelated to this change, we should probably do a follow-up to the follow-up to fix the Property 'StarlightThemeProvider' does not exist on type 'Window & typeof globalThis'.ts(2339) TS error, I would guess it's just missing an export for declare in packages/starlight/global.d.ts.

I could do that in this PR I think — you’re right that export declare ... fixes the error on my end.

@delucis delucis added the 🌟 patch Change that triggers a patch release label Apr 10, 2024
@delucis delucis merged commit 4493dcf into main Apr 10, 2024
9 checks passed
@delucis delucis deleted the chris/theme-select branch April 10, 2024 20:10
@astrobot-houston astrobot-houston mentioned this pull request Apr 10, 2024
@techfg
Copy link
Contributor

techfg commented Apr 11, 2024

@delucis - Great stuff here and thank you! I had mis-interpreted the previous logic when making the media change mod in that I thought the double event listener issue already existed for the select itself but realize now that the event listener that existed on the select wasn't doubled up since it was instance specific, so my change did introduce the new bug - it didn't exist prior. Sorry I overlooked that originally and again, appreciate you getting this cleaned-up!

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 🌟 patch Change that triggers a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants