Skip to content

useTheme hooks changes for Auto option#35

Open
neural-machine wants to merge 2 commits into
mainfrom
neural/auto-theme-option
Open

useTheme hooks changes for Auto option#35
neural-machine wants to merge 2 commits into
mainfrom
neural/auto-theme-option

Conversation

@neural-machine

@neural-machine neural-machine commented Aug 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Junoblocks changes for Auto-theme option.

@vercel

vercel Bot commented Aug 2, 2022 •
edited
Loading

Copy link
Copy Markdown

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

Name Status Preview Updated
junoblocks ✅ Ready (Inspect) Visit Preview Aug 9, 2022 at 11:27PM (UTC)

@sashimi36

sashimi36 commented Aug 2, 2022

Copy link
Copy Markdown
Owner

Nice work. Do we keep useSubscribeToDefaultAppTheme still or want to cut it off somehow?

@neural-machine

neural-machine commented Aug 9, 2022

Copy link
Copy Markdown
Collaborator Author

We should probs cut the useSubscribeToDefaultAppTheme. Sorry I always forget to check Github notifications.

sashimi36
sashimi36 reviewed Aug 10, 2022
View reviewed changes
Comment thread src/theme/hooks/useTheme.ts
setLightTheme(enabled: boolean) {
setTheme((prev) => {
const fallbackTheme =
prev.theme !== getLightTheme(prev.themes)

@sashimi36 sashimi36 Aug 10, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@neural-machine how do we know if a theme is light or dark?

@neural-machine neural-machine Aug 18, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry! I completely forgot about this PR.

Right now, light and dark theme has name light and dark respectively. This invariant was here before the change so it's not a breaking change.

@sashimi36 sashimi36 Aug 24, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@neural-machine think it would be best if we were to have a new field kind or type to indicate if the type of a given theme is light or dark accordingly. I would also look into ways of defining a default light/dark themes by providing a flag and applying them when relying on the OS setting.

@neural-machine
neural-machine marked this pull request as ready for review August 18, 2022 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sashimi36 sashimi36 Awaiting requested review from sashimi36

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@neural-machine @sashimi36 @baoskee