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

🍪 Use localStorage instead of cookie for static builds' theme #445

Merged
merged 16 commits into from
Sep 9, 2024

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Aug 5, 2024

This PR doesn't remove the cookie pathway, but rather augments it for static builds. We shouldn't end up with a cookie in these contexts.

Fixes #319
Fixes jupyter-book/mystmd#585

Also fixes bug in which system preference is ignored.

Live MyST server:

2024-08-27.11-41-48.mp4

Static MyST HTML build:

2024-08-27.11-42-21.mp4

Copy link

changeset-bot bot commented Aug 5, 2024

🦋 Changeset detected

Latest commit: d4b6d63

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

This PR includes changesets to release 12 packages
Name Type
@myst-theme/providers Minor
@myst-theme/site Minor
@myst-theme/diagrams Minor
@myst-theme/jupyter Minor
myst-to-react Minor
@myst-theme/article Minor
@myst-theme/book Minor
@myst-theme/frontmatter Minor
@myst-theme/styles Minor
@myst-theme/common Minor
@myst-theme/icons Minor
myst-demo Minor

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

@agoose77
Copy link
Collaborator Author

agoose77 commented Aug 5, 2024

@agoose77 agoose77 requested a review from rowanc1 August 5, 2024 21:21
@agoose77 agoose77 changed the title 🍪 Use localStorage instead of cookie for static builds 🍪 Use localStorage instead of cookie for static builds' theme Aug 5, 2024
@rowanc1 rowanc1 self-assigned this Aug 20, 2024
@agoose77 agoose77 marked this pull request as draft August 26, 2024 20:45
@agoose77 agoose77 force-pushed the agoose77/fix-local-cookie-theme branch from 08621a8 to 634a8e7 Compare August 27, 2024 09:24
@agoose77
Copy link
Collaborator Author

Wow, that was a bit of a journey. I think I've learned a lot more about React and SSR.

The challenging part of this puzzle was dealing with client-side state (matchMedia and localStorage) in a way that avoids a FOUC due to a client-only re-render.

The way that the above guide handles dark-mode is to defer responsibility for choosing the theme to the client in cases where we don't have an SSR_available theme. When the client hydrates, it can read the client-side APIs to determine the proper value of theme. However, this means that the SSR result may not correctly hydrate with the intended state on the client. There are two ways to deal with this:

  1. Ensure the initial hydration matches SSR and trigger a re-render (e.g. using useEffect)
  2. Modify the DOM in-place before hydration such that it hydrates properly

(2) is the approach taken in the core theme logic that sets the tailwind class on the html node. We only need to do this if we don't know the SSR-determined theme, which occurs whenever a route is loaded without a set cookie, i.e. for every static build page load, and any dynamic SPA load whereby the user has not toggled the theme before.

We also need to contend with the theme selector which has to reconcile the same problem. I opted to remove any theme-dependence of the DOM here, so the tooltips, aria labels, and SVGs are invariant, and tailwind is left in charge of handling appearance.

Another

@agoose77 agoose77 marked this pull request as ready for review August 27, 2024 10:20
@agoose77 agoose77 force-pushed the agoose77/fix-local-cookie-theme branch 2 times, most recently from 3481676 to 52a3db7 Compare August 27, 2024 10:33
@agoose77
Copy link
Collaborator Author

One point to note; this means that the useTheme hook potentially is not very useful in our application; we need to handle the initial state loading on the client in a special manner. I don't know if there is a way to indicate this in our API so that we don't accidentally use const { theme } = useTheme() and find ourselves surprised that it doesn't work as expected for static builds.

Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

These changes are going to conflict with the choices made in downstream themes. I think there are a few small tweaks that we can make to ensure the logic/components for these choices is made in site, not providers.

Let me know if that makes sense, or I can expand on what I mean further..!

@agoose77 agoose77 force-pushed the agoose77/fix-local-cookie-theme branch from db92e3b to 0b728ce Compare August 29, 2024 11:04
@agoose77
Copy link
Collaborator Author

agoose77 commented Aug 29, 2024

@rowanc1 I've reworked the logic such that the providers are very thin, and all of the business logic is really in a new hooks subdirectory in site. Consequently, it breaks some existing APIs by changing their signatures. Does this work better for you?

@rowanc1
Copy link
Member

rowanc1 commented Aug 29, 2024

Thank you yes, that looks solid - I will take a look/test this afternoon and aim to get it in!

Copy link
Collaborator Author

@agoose77 agoose77 left a comment

Choose a reason for hiding this comment

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

Removing change to cookie

@rowanc1
Copy link
Member

rowanc1 commented Aug 31, 2024

I will fix up the conflicts and merge on Monday. Sorry for my delay.

@agoose77 agoose77 force-pushed the agoose77/fix-local-cookie-theme branch from e6adf6e to 46b414d Compare September 3, 2024 11:53
@agoose77 agoose77 force-pushed the agoose77/fix-local-cookie-theme branch from 46b414d to cd3c0ab Compare September 3, 2024 11:53
@stevejpurves
Copy link
Contributor

did some final regression checks on non-static sites, looks good, merging!

@stevejpurves stevejpurves dismissed rowanc1’s stale review September 9, 2024 11:25

Looks like @rowca1's concerns were addressed, but this flag just remains

@stevejpurves stevejpurves merged commit a76b8f1 into main Sep 9, 2024
2 checks passed
@stevejpurves stevejpurves deleted the agoose77/fix-local-cookie-theme branch September 9, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
3 participants