Skip to content

Conversation

@sfc-gh-tteixeira
Copy link
Contributor

@sfc-gh-tteixeira sfc-gh-tteixeira commented Nov 22, 2024

Make st.write use st.html when repr_html is present.

Describe your changes

  1. When _repr_html_() is present in an object, st.write now displays the HTML using st.html() rather than st.markdown() (assuming unsafe_allow_html=True).

  2. When unsafe_allow_html=False AND _repr_html_() did not contain HTML tags we used to send its output to st.markdown(). This behavior is now removed. Instead, the output is handled with st.help() like any other object.

  3. Removes _repr_html_() from *Connection objects. This wasn't really buying us anything, and was just weird. Also, this was the reason for adding behavior Improve caching code #2 in the first place.

  4. This is totally unrelated to the above, but is so small I didn't want to create a separate PR for it (though, LMK if you want me to!): Remove an old check for an environment variable that Streamlit's Atom plugin from 2018-19 used to depend on. That plugin no longer exists, so we can safely remove this.

GitHub Issue Link (if applicable)

n/a

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python): ✅ Included!
  • E2E Tests: 🟢 No new tests added, but this is already well-tested.
  • Any manual testing needed? 👎 No need.

Contribution License Agreement

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

This also changes some behavior:

1. When unsafe_allow_html=False AND _repr_html_() did not contain HTML tags
we used to send its output to st.markdown(). This behavior is now removed.
Instead, the output is handled like a normal object (that is, with st.help).

2. Removes _repr_html_ from *Connection objects. This didn't really buy us
anything, and was just weird. Also, this was the reason for adding behavior #1
in the first place.
@sfc-gh-tteixeira
Copy link
Contributor Author

FYI: We talked about these changes here

@kmcgrady kmcgrady added security-assessment-completed Security assessment has been completed for PR change:bugfix PR contains bug fix implementation impact:users PR changes affect end users labels Nov 26, 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.

Overall looks good. You might need to rebase the branch to get the latest playwright changes to work properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These screenshots are just random issues and are not tied to the change to html, right? It doesn't seem like it should be calling html on this, but I guess I didn't expect these screenshots. Just wanted to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what it looks like. But a merge didn't help. Super weird.

Will look into it in a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can't tell what could be causing it. These just seem to be flakes.

Since st_write_test.py was breaking often, I split it into multiple files and it seems to be better now. But then st.video_test.py started failing 🤷‍♂️

@kmcgrady kmcgrady merged commit 103a0c2 into develop Dec 11, 2024
32 checks passed
@kmcgrady kmcgrady deleted the reprhtml branch December 11, 2024 22:58
edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025
Make st.write use st.html when _repr_html_ is present.

## Describe your changes

1. When `_repr_html_()` is present in an object, `st.write` now displays
the HTML using `st.html()` rather than `st.markdown()` (assuming
`unsafe_allow_html=True`).

1. When `unsafe_allow_html=False` AND `_repr_html_()` did not contain
HTML tags we used to send its output to `st.markdown()`. This behavior
is now removed. Instead, the output is handled with `st.help()` like any
other object.

1. Removes `_repr_html_()` from `*Connection` objects. This wasn't
really buying us anything, and was just weird. Also, this was the reason
for adding behavior #2 in the first place.

1. This is totally unrelated to the above, but is so small I didn't want
to create a separate PR for it (though, LMK if you want me to!): Remove
an old check for an environment variable that Streamlit's Atom plugin
from 2018-19 used to depend on. That plugin no longer exists, so we can
safely remove this.


## GitHub Issue Link (if applicable)
n/a

## Testing Plan

- ~~Explanation of why no additional tests are needed~~
- Unit Tests (JS and/or Python): ✅ Included!
- E2E Tests: 🟢  No new tests added, but this is already well-tested.
- Any manual testing needed? 👎 No need.

---

**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

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.

3 participants