-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow setting placeholder for st.multiselect #6901
Allow setting placeholder for st.multiselect #6901
Conversation
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.
Thanks for the contribution @fhiroki!
The code here mostly looks good to me, but I left a few small comments with some things that should be addressed. Additionally, it'd be great to add a few tests in lib/tests/streamlit/elements/multiselect_test.py
and frontend/src/lib/components/widgets/Multiselect/Multiselect.test.tsx
that ensure that the default values are set as expected and can be changed. An e2e test in e2e/scripts/st_multiselect.py
and e2e/specs/st_multiselect.spec.js
would also be nice to have.
Another thing is that, aside from the code itself, we'll want to be sure that we want to do this from a product perspective. Adding this feature seems reasonable to me, but I'll defer to @jrieke and @sfc-gh-jcarroll for a final decision on adding new parameters to the st.multiselect
API.
frontend/src/lib/components/widgets/Multiselect/Multiselect.tsx
Outdated
Show resolved
Hide resolved
Nice, thanks for the contribution. Couple questions / comments:
|
@vdonato |
@sfc-gh-jcarroll
Since st.selectbox requires some kind of selection by default, the need for a placeholder is less than st.multiselect. But as you say, for consistency, we can add it to st.selectbox as well.
I apologize for making a suggestion that goes against the design philosophy of Streamlit. However, it would be very beneficial if it could be set on the user side, since On the other hand,
I guess this parameter is for when the options list is empty at the start of Streamlit. Therefore, I don't think it is usually empty. Removed this param with the following commit. |
We could also allow that all via the placeholder: str | tuple[str, str] | tuple[str, str, str]
...
# Only set no selection placeholder:
placeholder = "Select options"
# Set no selection and no results placeholder:
placeholder = ("Select Options", "No results")
# Set no selection, no results and no options placeholder:
placeholder = ("Select Options", "No results", "No options") This will make it a bit less discoverable that these placeholders can be updated as well, but no results / no options are anyways more niche use cases. @sfc-gh-jcarroll What do you think? |
Super cool, thanks. I think I'm fine with merging this with a few caveats:
|
Yep makes sense to add selectbox support later when it's possible. My preference as well is to keep one parameter that just takes a string for placeholder, and keep "No results" as a constant for now. One other use case for changing "No results" is localization / non-English apps but I think it would need a bigger discussion so I would prefer to not handle it one-off here. |
Thanks for adding the unit tests @fhiroki! I'm happy to help with adding some e2e tests once we've finalized the API for this (note that I'll be out of the office from Thursday to July 5, so I may be delayed in being able to help with this). I like @lukasmasuch's suggestion with
I don't think parameter order for kwarg-only parameters actually has any effect aside from what order the parameters will appear in the documentation, so we should be able to shuffle these around easily. |
Thank you all for your suggestions. It seems strongly desired to have only
This makes a lot of sense, since the parameters close to the user must be carefully selected or the cognitive load will be high.
Actually, I was thinking of adding
@vdonato Thank you for your kindness. With the simpler requirements, I'm wondering if I just modify the existing tests a bit. 071e0ed In a separate PR, I would be happy to add a placeholder to st.selectbox too. Please let me know if it is more convenient to do this within this PR. |
@fhiroki adding the selectbox placeholder in a separate PR would be the preferred path. Then we can already merge this PR for multiselect. |
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.
With the simpler requirements, I'm wondering if I just modify the existing tests a bit. 071e0ed
I think that should be okay given that the changes are now simpler.
This LGTM! We'll just need to make some small changes to the proto file to ensure that schema versions are backwards compatible, but aside from that this PR is ready to be merged.
The max selections that can be selected at a time. | ||
This argument can only be supplied by keyword. | ||
placeholder : str | ||
An optional string to display when option is not selected. |
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.
nit: maybe "A string to display when no options are selected. Defaults to 'Choose an option'"?
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.
d14d375
Thank you for the suggestion. It would be better to specify the default value.
int32 max_selections = 9; | ||
string placeholder = 10; | ||
bool disabled = 11; | ||
LabelVisibilityMessage label_visibility = 12; |
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.
Same comment here as in #6913 (comment) we'll need to keep tag numbers the same for existing fields and add placeholder
as tag 12.
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.
d14d375
I understood that it is not necessary to match the order of the arguments. Thanks for telling me.
@fhiroki Thanks for the changes. Based on some changes we just merged into develop, you need to merge with develop as well and resolve some conflicts. |
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, thanks @fhiroki! I can handle merging the latest changes from develop
to get rid of the merge conflicts.
* develop: Feature: st.toast (streamlit#6783) Bump semver from 5.7.1 to 5.7.2 in /frontend (streamlit#6982) Add host communication e2e tests (streamlit#6806) Re-add homepage to package.json (streamlit#6987) Remove unnecessary code and Add rm commands to make clean (streamlit#6980) Allow setting placeholder for st.selectbox (streamlit#6913) Allow setting placeholder for st.multiselect (streamlit#6901) Also use bottom padding in embedded mode for chat input (streamlit#6979) Feature/st lib (streamlit#6692) Remove unused import (streamlit#6977) Release 1.24.1 (streamlit#6965) Update st.audio/st.video docstrings (streamlit#6964) Slightly simplify bug report template (streamlit#6972) Fix baseweb warnings by using longhand properties (streamlit#6976)
* multiselect placeholders can be set * destructure placeholder and placeholderNoOptions * fix placeholder type Optional[str] to str * remove placeholder_no_options * add unit test * remove placeholder_no_results and change order of parameters * change the order of args * update e2e test * revert proto number and fix docs --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>
* multiselect placeholders can be set * destructure placeholder and placeholderNoOptions * fix placeholder type Optional[str] to str * remove placeholder_no_options * add unit test * remove placeholder_no_results and change order of parameters * change the order of args * update e2e test * revert proto number and fix docs --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>
* multiselect placeholders can be set * destructure placeholder and placeholderNoOptions * fix placeholder type Optional[str] to str * remove placeholder_no_options * add unit test * remove placeholder_no_results and change order of parameters * change the order of args * update e2e test * revert proto number and fix docs --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>
* multiselect placeholders can be set * destructure placeholder and placeholderNoOptions * fix placeholder type Optional[str] to str * remove placeholder_no_options * add unit test * remove placeholder_no_results and change order of parameters * change the order of args * update e2e test * revert proto number and fix docs --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>
* multiselect placeholders can be set * destructure placeholder and placeholderNoOptions * fix placeholder type Optional[str] to str * remove placeholder_no_options * add unit test * remove placeholder_no_results and change order of parameters * change the order of args * update e2e test * revert proto number and fix docs --------- Co-authored-by: Vincent Donato <vincent@streamlit.io>
Describe your changes
As described here,
st.multiselect
widgets cannot change placeholders.CSS hacks solve the problem, but it is tricky, and will not work if the DOM structure changes in the future.
Therefore, I would like to be able to change placeholders in the official way.
GitHub Issue Link (if applicable)
#4750
Testing Plan
lib/streamlit/hello/Hello.py
to set placeholders in local and it would change to the appropriate placeholder.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.