Skip to content

Conversation

@willhuang1997
Copy link
Contributor

@willhuang1997 willhuang1997 commented Dec 1, 2022

📚 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?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

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

@willhuang1997 willhuang1997 marked this pull request as ready for review December 1, 2022 08:46
Copy link
Collaborator

@lukasmasuch lukasmasuch left a 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})
Copy link
Collaborator

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

Copy link
Contributor Author

@willhuang1997 willhuang1997 Dec 5, 2022

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.

Copy link
Collaborator

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 👍

Copy link
Contributor Author

@willhuang1997 willhuang1997 Dec 9, 2022

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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').

@willhuang1997 willhuang1997 merged commit 9ef84f6 into streamlit:develop Dec 9, 2022
tconkling added a commit that referenced this pull request Dec 20, 2022
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants