Skip to content

Conversation

@sfc-gh-jgarcia
Copy link
Contributor

@sfc-gh-jgarcia sfc-gh-jgarcia commented Jun 12, 2023

Describe your changes

This PR fixes the issue reported here by @sfc-gh-wschmitt where some elements of the Streamlit UI won't change their font family when using a postMessage theming call in SiS.

A piece of this was fixed by @sfc-gh-tteixeira here, but a few things were still un-styled, especially in the sidebar: This PR fixes that.

Before:

Kapture.2023-06-05.at.14.16.12.mp4
Screenshot 2023-05-22 at 13 35 51 Screenshot 2023-05-22 at 13 36 04 Screenshot 2023-05-22 at 13 36 42

After:

Kapture.2023-06-09.at.16.45.56.mp4

Screenshot 2023-06-05 at 1 47 50 PM

Jira Issue Link

Testing Plan


Contribution License Agreement

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

@sfc-gh-jgarcia sfc-gh-jgarcia added change:bugfix PR contains bug fix implementation impact:users PR changes affect end users security-assessment-completed Security assessment has been completed for PR labels Jun 12, 2023
@sfc-gh-wschmitt
Copy link

Exciting 🎉

@sfc-gh-jgarcia
Copy link
Contributor Author

One clarification point is: For st.dataframe, there's an issue where the dataframe will keep the default font and will flicker to the new font after interaction, as can be seen here 👇

Kapture.2023-06-05.at.14.16.12.mp4

This seems to be related to this widget being a <canvas> element. A few alternatives we discussed with @sfc-gh-tteixeira are:

  • We could try to force a re-render by passing a dummy prop to the dataframe component, which changes when the theme changes;
  • We could try to tell the underlying dataframe library we’re using to refresh somehow. Need to reach their docs.

However, we discussed this with @sfc-gh-wschmitt and agreed that for now it won't matter in practice because the theme postMessage call would arrive before anything else is drawn on the screen. At some point we might want to address this differently so we don't run into race conditions if we get really good at loading SiS apps, but for now we should be good to ship this with the next Streamlit release in SiS.

@sfc-gh-jgarcia sfc-gh-jgarcia marked this pull request as ready for review June 12, 2023 18:54
@sfc-gh-wihuang sfc-gh-wihuang self-assigned this Jun 13, 2023
Copy link
Contributor

@sfc-gh-wihuang sfc-gh-wihuang left a comment

Choose a reason for hiding this comment

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

Are there any other props that need to get passed here? I assume no but just want to double check and ask again.

@sfc-gh-jgarcia
Copy link
Contributor Author

Are there any other props that need to get passed here? I assume no but just want to double check and ask again.

Not for now, we should be fine with this solution until we get really good at loading SiS apps, which won't happen anytime soon 😄

@sfc-gh-wihuang sfc-gh-wihuang merged commit 8d86fdd into develop Jun 20, 2023
@mayagbarnes mayagbarnes deleted the sis-theming-fixes branch June 20, 2023 17:45
tconkling added a commit to tconkling/streamlit that referenced this pull request Jun 20, 2023
* tim/MyPy14:
  whoops
  Fix mypy errors
  Fix on sidebar elements not changing its font (streamlit#6825)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:bugfix PR contains bug fix implementation impact:users PR changes affect end users security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants