Skip to content

Add Dark theme support#150

Open
dmitrysimkin wants to merge 7 commits intoBarredEwe:mainfrom
dmitrysimkin:5.2.0
Open

Add Dark theme support#150
dmitrysimkin wants to merge 7 commits intoBarredEwe:mainfrom
dmitrysimkin:5.2.0

Conversation

@dmitrysimkin
Copy link
Copy Markdown

Short description 📝

Add support for Dark mode theme.
When config is set to record dark mode images as well it will do so, record dark and light images

Solution 📦

  • Add config
  • Updated template to respect dark mode flag

Implementation 👩‍💻👨‍💻

It's a bit raw now, just want to get feedback @BarredEwe and then if fine I can cleanup and add tests

@BarredEwe
Copy link
Copy Markdown
Owner

Thanks for pushing this. The direction makes sense, but I see a few blocking issues before this can go in:

  1. The dark-mode snapshots currently use the same snapshot name as the light-mode ones. That means they will either overwrite the light reference when recording or compare against the light baseline and fail. The dark pass needs a distinct test/snapshot name, for example with a -dark suffix.

  2. This changes the public PrefireSnapshot API in a source-breaking way. The existing closure-based initializers were replaced with value-based ones, so consumer code like PrefireSnapshot({ MyView() }, ...) will stop compiling. If this is not intended as a breaking release, the old overloads should stay and just gain isDark.

  3. The non-dark path now forces light mode. That changes existing behavior for previews that explicitly opt into dark mode or depend on the ambient color scheme. I think only the dark pass should force a color scheme; the default pass should preserve current behavior.

I also do not see tests covering the new dark-mode generation path yet. Please add coverage for:

  • generating the extra dark snapshot assertion when record_in_dark_mode is enabled
  • distinct snapshot naming for dark mode
  • preserving the existing public PrefireSnapshot API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants