Skip to content

Conversation

@kajarenc
Copy link
Collaborator

@kajarenc kajarenc commented Nov 15, 2022

📚 Context

Add the label_visibility keyword-only parameter to st.checkbox widget, to be consistent with other widgets.

  • What kind of change does this PR introduce?

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

🧠 Description of Changes

Please note that unlike other widgets st.checkbox doesn't use WidgetLabel component for displaying widgets. That's why we have a bit different implementation for this case on the frontend side. Backend changes are identical to other widgets.

  • This is a breaking API change
  • This is a visible (user-facing) change

Visible:
Screen Shot 2022-11-16 at 18 57 51

Hidden:
Screen Shot 2022-11-16 at 18 58 52

Collapsed:
Screen Shot 2022-11-16 at 19 00 11

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

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


Contribution License Agreement

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

@kajarenc kajarenc changed the title Add label_visibility option for st.checkbox [WIP] Add label_visibility option for st.checkbox Nov 15, 2022
@kajarenc kajarenc changed the title [WIP] Add label_visibility option for st.checkbox Add label_visibility option for st.checkbox Nov 16, 2022
@kajarenc kajarenc marked this pull request as ready for review November 16, 2022 15:13
@kajarenc kajarenc requested a review from mayagbarnes November 16, 2022 15:14
c = self.get_delta_from_queue().new_element.checkbox
self.assertEqual(c.label, "the label")
self.assertEqual(c.label_visibility.value, proto_value)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to write a test for the 3rd scenario where label="" and the warning is raised?

Copy link
Collaborator Author

@kajarenc kajarenc Nov 17, 2022

Choose a reason for hiding this comment

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

Test added!

checked={this.state.value}
disabled={disabled}
onChange={this.onChange}
aria-label={element.label}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch 👍🏼

Copy link
Collaborator

@mayagbarnes mayagbarnes left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍🏼
Just curious if label_visibility is being considered for st.metric as well?

Think proto change requires addtl sign off from Lukas, Vincent, or Tim

@kajarenc kajarenc merged commit 1cdbb65 into streamlit:develop Nov 17, 2022
tconkling added a commit to tconkling/streamlit that referenced this pull request Nov 18, 2022
* develop: (25 commits)
  Fix CORS acronym in docstring (streamlit#5727)
  Add integration tests for Snowpark (streamlit#5543)
  Release/1.15.0 (streamlit#5720)
  Add audit_frontend_dependencies script to CODEOWNERS (streamlit#5708)
  Add label_visibility option for st.checkbox (streamlit#5705)
  Display existing column names in st.map exception and make st.map work with capital letters (streamlit#5679)
  Plotly Customization (streamlit#5681)
  Turn off theme for now (streamlit#5701)
  Add all vendored code to `make notices` (streamlit#5704)
  Audit frontend licenses (streamlit#5664)
  Surround labels in quotes (streamlit#5703)
  Add info about voting on features (streamlit#5660)
  Update issue labeling scheme to adopt new standards (streamlit#5702)
  Cached media (audio+video) replay (streamlit#5695)
  Fix docstring line wrap (streamlit#5698)
  Use specialized assertion functions (streamlit#5680)
  Release 1.14.1 (streamlit#5693)
  Image replay in cached functions (streamlit#5675)
  Add docstrings for `experimental_allow_widgets` (streamlit#5670)
  Remove `st.write` from `CachedStFunctionWarning` (streamlit#5669)
  ...
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.

Implement label_visibility with Checkbox

3 participants