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.

Sign up for GitHub

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

Jump to bottom

Add st.status container #7140

Merged
merged 43 commits into from Aug 18, 2023
Merged

Add st.status container #7140

merged 43 commits into from Aug 18, 2023

Conversation

LukasMasuch
Copy link
Collaborator

@LukasMasuch LukasMasuch commented Aug 9, 2023 •
edited

Describe your changes

This PR adds st.status, a new layout container focused on displaying output from long-running tasks and processes. This PR will also update the old implementation used for the langchain callback handler.

Testing Plan

  • Unit Tests: JS: ✅ Python: ✅
  • E2E Tests ✅

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-jcarroll sfc-gh-jcarroll mentioned this pull request Aug 10, 2023
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 15, 2023
View reviewed changes
lib/streamlit/elements/layouts.py Dismissed Show dismissed Hide dismissed
lib/streamlit/elements/lib/mutable_status_container.py Fixed Show fixed Hide fixed
@LukasMasuch LukasMasuch changed the title [WIP] Add st.status container Add st.status layout container Aug 15, 2023
@LukasMasuch LukasMasuch marked this pull request as ready for review August 15, 2023 19:03
@LukasMasuch LukasMasuch changed the title Add st.status layout container Add st.status container Aug 16, 2023
willhuang1997
willhuang1997 approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@willhuang1997 willhuang1997 left a comment

Choose a reason for hiding this comment

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

Left some comments about the tests. Implementation looks good

vdonato
vdonato approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

+1 for the proto changes (note I didn't give the rest of the code a thorough review)

proto/streamlit/proto/Block.proto
@@ -44,7 +44,8 @@ message Block {

message Expandable {
string label = 1;
bool expanded = 2;
optional bool expanded = 2;
Copy link
Collaborator

@vdonato vdonato Aug 16, 2023 •
edited

Choose a reason for hiding this comment

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

is the addition of the optional keyword needed here? fields in proto3 are always technically optional, so I'm not sure if this makes any real difference (+ I'm not entirely sure if it can somehow affect backwards-compatibility-related things but don't see why it would hurt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While fields are indeed optional as default, they are not nullable (as far as I understand this). For the update functionality, we need to differentiate between expanded being provided (False or True) or it being absence (== null) in which case we don't modify the expanded state in the frontend. Therefore, I think we need to use optional in this case. I also came to a similar conclusion on backwards-compatibility; not 100% sure, but I couldn't find a reason why it could be problematic.

@LukasMasuch LukasMasuch merged commit 4061392 into develop Aug 18, 2023
86 checks passed
@LukasMasuch LukasMasuch deleted the feature/mutable-expander-v2 branch August 24, 2023 18:27
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
@LukasMasuch @sfc-gh-dmatthews @eric-skydio
Add st.status container (streamlit#7140)
accf01d 
* Add icon to expander

* Add implementation for st.status

* Fix

* Add top comment

* Add docstring

* Fixes

* Improve implementation

* Some refactoring

* Minor fixes

* Add auto collapse mode

* Add auto collapse mode

* Various fixes and improvements

* Add empty expander test

* Add empty expander test

* Disallow empty for normal expander

* Apply various updates

* Some refactoring

* Update status container

* Update callback handler

* Change empty state for expander

* Move icon implementation to expander

* Add e2e tests

* Add unit tests

* Add empty expander e2e test

* Update comment

* Cleanups

* Cleanup

* Add metrics

* Update docstring

* Update snapshot

* Add test for empty status

* Use smaller block

* Update docstrings

* Don't change color on hover if empty

* Update docstrings based on feedback

* Minor change

* Update playback tests

* Integrated PR feedback

* Fix test

* Use descriptive names for screenshot tests

* Update status snapshots

* Update st.status docstring

---------

Co-authored-by: Debbie Matthews <debbie.matthews@snowflake.com>
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
@LukasMasuch @sfc-gh-dmatthews
Add st.status container (streamlit#7140)
4926765 
* Add icon to expander

* Add implementation for st.status

* Fix

* Add top comment

* Add docstring

* Fixes

* Improve implementation

* Some refactoring

* Minor fixes

* Add auto collapse mode

* Add auto collapse mode

* Various fixes and improvements

* Add empty expander test

* Add empty expander test

* Disallow empty for normal expander

* Apply various updates

* Some refactoring

* Update status container

* Update callback handler

* Change empty state for expander

* Move icon implementation to expander

* Add e2e tests

* Add unit tests

* Add empty expander e2e test

* Update comment

* Cleanups

* Cleanup

* Add metrics

* Update docstring

* Update snapshot

* Add test for empty status

* Use smaller block

* Update docstrings

* Don't change color on hover if empty

* Update docstrings based on feedback

* Minor change

* Update playback tests

* Integrated PR feedback

* Fix test

* Use descriptive names for screenshot tests

* Update status snapshots

* Update st.status docstring

---------

Co-authored-by: Debbie Matthews <debbie.matthews@snowflake.com>
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
@LukasMasuch @sfc-gh-dmatthews @zyxue
Add st.status container (streamlit#7140)
bc1ea7d 
* Add icon to expander

* Add implementation for st.status

* Fix

* Add top comment

* Add docstring

* Fixes

* Improve implementation

* Some refactoring

* Minor fixes

* Add auto collapse mode

* Add auto collapse mode

* Various fixes and improvements

* Add empty expander test

* Add empty expander test

* Disallow empty for normal expander

* Apply various updates

* Some refactoring

* Update status container

* Update callback handler

* Change empty state for expander

* Move icon implementation to expander

* Add e2e tests

* Add unit tests

* Add empty expander e2e test

* Update comment

* Cleanups

* Cleanup

* Add metrics

* Update docstring

* Update snapshot

* Add test for empty status

* Use smaller block

* Update docstrings

* Don't change color on hover if empty

* Update docstrings based on feedback

* Minor change

* Update playback tests

* Integrated PR feedback

* Fix test

* Use descriptive names for screenshot tests

* Update status snapshots

* Update st.status docstring

---------

Co-authored-by: Debbie Matthews <debbie.matthews@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willhuang1997 willhuang1997 willhuang1997 approved these changes

@sfc-gh-jcarroll sfc-gh-jcarroll sfc-gh-jcarroll left review comments

@vdonato vdonato vdonato approved these changes

@sfc-gh-mnowotka sfc-gh-mnowotka Awaiting requested review from sfc-gh-mnowotka sfc-gh-mnowotka is a code owner

Assignees
No one assigned
Labels
change:feature impact:users security-assessment-completed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@LukasMasuch @vdonato @willhuang1997 @sfc-gh-jcarroll @sfc-gh-dmatthews