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 white components backgrounds when OS is using dark theme #8242

Merged
merged 7 commits into from Mar 18, 2024

Conversation

LukasMasuch
Copy link
Collaborator

@LukasMasuch LukasMasuch commented Mar 6, 2024

Describe your changes

I previously attempted to fix this issue in #7821. This fix worked fine, but only if the OS isn't configured to dark mode (+ Chrome). This fix sets the color scheme to normal to indicate that the element isn't aware of any color schemes. This seems to provide us with the desired behaviour to not automatically show a white background.

GitHub Issue Link (if applicable)

Closes #8156
Closes #7813

Testing Plan

  • The test should be fine to cover this.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@LukasMasuch LukasMasuch marked this pull request as ready for review March 6, 2024 03:41
@LukasMasuch LukasMasuch changed the title Fix white backgrounds of components when OS is using dark theme Fix white components backgrounds when OS is using dark theme Mar 6, 2024
Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Making sure I understand, we are effectively unsetting the color scheme (cause normal is the default and color-scheme is inherited).

Does this seem more helpful to just say that all iframes have color-scheme: normal in the global styles? Just avoids the need to remember this for the next time?

e2e_playwright/st_components_v1_test.py Show resolved Hide resolved
@LukasMasuch
Copy link
Collaborator Author

Making sure I understand, we are effectively unsetting the color scheme (cause normal is the default and color-scheme is inherited).

Yep, exactly 👍

Does this seem more helpful to just say that all iframes have color-scheme: normal in the global styles? Just avoids the need to remember this for the next time?

yeah, might make sense but I need to think a bit more about that. There could be some potential unexpected side-effects if we just apply this to all iframes. Another way to improve this might be to share a customized Iframe component between multiple elements. There are other properties that might make sense to have shared across multiple iframe uses.

@LukasMasuch LukasMasuch merged commit ef2851f into develop Mar 18, 2024
35 of 36 checks passed
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
…it#8242)

## Describe your changes

I previously attempted to fix this issue in
streamlit#7821. This fix worked fine,
but only if the OS isn't configured to dark mode (+ Chrome). This fix
sets the color scheme to `normal` to indicate that the element isn't
aware of any color schemes. This seems to provide us with the desired
behaviour to not automatically show a white background.

## GitHub Issue Link (if applicable)

Closes streamlit#8156
Closes streamlit#7813

## Testing Plan

- The test should be fine to cover this. 

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants