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

fix default values for system-theme-overrides #20464

Merged
merged 2 commits into from
Nov 17, 2023

Conversation

azrikahar
Copy link
Contributor

@azrikahar azrikahar commented Nov 17, 2023

Context

Currently when a user with light theme looks at Dark Theme Customizations field, the default values are actually light theme's rather than dark theme's:

(the foreground for dark theme should be light/white, while the background should be black/dark)

This is because darkMode here depends on the current user's configured appearance, rather than "light" for Light Theme Customization, and "dark" for Dark Theme Customization:

const { darkMode, themeDark, themeLight } = useThemeConfiguration();

This PR adds a new prop to system-theme-overrides so it will use the forced/correct appearance instead of depending on the current user's appearance to show the correct default values.

Result

Scope

What's changed:

  • Use fixed dark/light appearance to set the correct default values for Light Theme Customizations and Dark Theme Customizations fields

Potential Risks / Drawbacks

  • None if I'm not mistaken, unless system-theme-overrides component will be used elsewhere where it needs to depend on the user's appearance

Review Notes / Questions

  • Check whether this logic is correct

Copy link

changeset-bot bot commented Nov 17, 2023

🦋 Changeset detected

Latest commit: 80c5b3d

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

This PR includes changesets to release 3 packages
Name Type
@directus/app Patch
@directus/api Patch
directus 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
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@paescuj paescuj merged commit 091d1a6 into main Nov 17, 2023
4 checks passed
@paescuj paescuj deleted the fix/system-theme-overrides-default-value branch November 17, 2023 22:53
@github-actions github-actions bot added this to the Next Release milestone Nov 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants