-
Notifications
You must be signed in to change notification settings - Fork 4k
Turn on streamlit theme for altair and plotly #5796
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
Conversation
lukasmasuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| STREAMLIT_THEME = {"embedOptions": {"theme": "streamlit"}} | ||
|
|
||
| # This allows to use alt.themes.enable("streamlit") to activate Streamlit theme. | ||
| alt.themes.register("streamlit", lambda: {"usermeta": STREAMLIT_THEME}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure if this is very useful with streamlit theme being activated as default. I think the only use is that if streamlit is activated as global theme, theme=None will also lead to using the streamlit theme, or?
Btw. can you add a screenshot test here to make sure that the global theming is doing what is expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. can you add a screenshot test here to make sure that the global theming is doing what is expected
Yep I'll add a test.
Hmm, not sure if this is very useful with streamlit theme being activated as default. I think the only use is that if streamlit is activated as global theme, theme=None will also lead to using the streamlit theme, or?
yes this is just to make it so streamlit can be activated on a global theme. Arnaud specifically wanted this so he didn't have to change his apps. If you have qualms about merging this, let me know and I can bring them up with Arnaud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably does not hurt to have it in, so I think it's fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went to go test this and tried doing alt.themes.enable('streamlit') and whenever I do a st_altair_chart(...), it will automatically turn on streamlit theme because streamlit is the default.
As for when do st_altair_chart(..., theme=None), this automatically removes the streamlit theme. It feels like there is no need to add a test for this command? Let me know if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but I think in this case, it's not really working as expected: I would expect that setting the streamlit theme globally (via alt.themes.enable('streamlit')) and using theme=None would result in the streamlit theme as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that theme=None just means that we are not applying/overwriting any theme but instead using the globally defined theme. Which would be streamlit in case of alt.themes.enable('streamlit').
* develop: Change workers balancing logic for e2e tests to always cover all specs (#5865) Up version to 1.16.0 (#5852) Stop installing pip-tools (#5854) Add eslint no relative import paths plugin to pre commit hooks (#5839) Docs: move note to `experimental_allow_widgets` param description (#5843) Update element docstrings for colored text support (#5831) Add remark-directive plugin (Support for colored text in markdown) (#5774) Teach WebsocketConnection how to wait on an external auth token (#5728) Turn on streamlit theme for altair and plotly (#5796) Fix markdown headers spacing (#5829) Update error message and docstring in st.map (#5792) Remove unnecessarily methods from DeltaGenerator (#5824) Resolve vega-tooltip to later version (#5825) Easter Egg (#5817) Update Exception Layout to avoid overflow (#5700)
📚 Context
This is to release plotly charts and altair charts with the streamlit theme. This turns it on for both of them.
This also reenables the altair theme to be set globally.
This PR should likely be checked through commits.
Please describe the project or issue background here
What kind of change does this PR introduce?
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.