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

feat(onboarding): Dark mode #21149

Closed
wants to merge 11 commits into from
Closed

feat(onboarding): Dark mode #21149

wants to merge 11 commits into from

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Mar 26, 2024

Problem

Dark mode is available on the website and in the logged-in part of the app, but not in the glue – login/signup/onboarding pages. Prompted by Zach's question about disabling dark mode UI snapshots, I thought it might be simpler to add the missing support for dark mode instead.

Changes

Bridge pages should now support dark mode. Additionally, this will be synced with posthog.com by default.

Does this work well for both Cloud and self-hosted?

👍

How did you test this code?

UI snapshots

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

30 snapshot changes in total. 0 added, 30 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Mar 26, 2024

Size Change: -369 B (0%)

Total Size: 824 kB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 824 kB -369 B (0%)

compressed-size-action

@Twixes Twixes requested review from benjackwhite and a team March 26, 2024 00:41
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

No complaints - will leave to the Growth team to decide if they want this (the whole reason we didn't do it was the concern that dark mode was more of a confusing UI to be in - especially without a toggle to get out of it 🤔

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

41 snapshot changes in total. 0 added, 41 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@zlwaterfield
Copy link
Contributor

No complaints - will leave to the Growth team to decide if they want this (the whole reason we didn't do it was the concern that dark mode was more of a confusing UI to be in - especially without a toggle to get out of it 🤔

@benjackwhite agree with you around the confusing UI but I do think it should be ok it:

  1. follows the same pattern as posthog.com
  2. uses the users system mode as default

I do want to make sure there will be no regression in the UI on these pages before merging.

@benjackwhite
Copy link
Contributor

No complaints - will leave to the Growth team to decide if they want this (the whole reason we didn't do it was the concern that dark mode was more of a confusing UI to be in - especially without a toggle to get out of it 🤔

@benjackwhite agree with you around the confusing UI but I do think it should be ok it:

  1. follows the same pattern as posthog.com
  2. uses the users system mode as default

I do want to make sure there will be no regression in the UI on these pages before merging.

@raquelmsmith was the one who raised this before essentially arguing that we should carefully AB test changes to the onboarding flow (but again I defer to you lot to decide 👍 )

userThemeMode: [
(s) => [s.user, s.themeFromCookie],
(user, themeFromCookie): UserTheme => {
return user?.theme_mode || themeFromCookie || 'light'
Copy link
Member

Choose a reason for hiding this comment

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

should be decently easy to set up a flag here I believe. Let's test it because we can, and because colors can have a big impact on signup.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Twixes how could we feature flag this just for the auth pages? I can do it if you'd like just needs some hints on where to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's this setFeatureFlags utility.

Example usage: https://github.com/PostHog/posthog/blob/master/frontend/src/layout/FeaturePreviews/FeaturePreviews.stories.tsx#L33

I haven't actually done this myself so this could be the wrong direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

added snapshot is somehow slightly cut off vertically?

Copy link
Contributor

Choose a reason for hiding this comment

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

PostHog Cloud logo needs a dark mode version

@@ -129,7 +129,8 @@ async function expectStoryToMatchSnapshot(
check = expectStoryToMatchComponentSnapshot
}

await waitForPageReady(page)
await waitForPageReady(page)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await waitForPageReady(page)
await waitForPageReady(page)

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@Twixes Twixes deleted the dark-mode-bridge branch June 13, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants