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.

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

docs: ui.combo_box #718

Merged
merged 16 commits into from
Aug 19, 2024
Merged

Conversation

AkshatJawne
Copy link
Contributor

Closes #687

plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

Ready for re-review @margaretkennedy

Copy link
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

Ready for re-review @bmingles @margaretkennedy

## API Reference

```{eval-rst}
.. dhautofunction:: deephaven.ui.combo_box
Copy link
Collaborator

@jnumainville jnumainville Aug 9, 2024

Choose a reason for hiding this comment

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

Since I see you have a new dhautofunction here, please either

  1. Create the docs locally and verify they are created correctly (the easiest way to do this is via tools/plugin_builder.py, check out the readme and let me know if you have problems).
  2. Wait until fix: Prevent pushing broken docs to main #719 is merged then merge main in to this branch so the docs will be checked automatically
    Thanks

@AkshatJawne AkshatJawne requested a review from bmingles August 13, 2024 19:10
@@ -323,27 +327,37 @@ my_combo_box_event_example = ui_combo_box_event_example()

## Control

When a combo box has multiple controlled properties (e.g., `input_value`, `selected_key`), updates to one property do not automatically update the others. Each interaction triggers only its specific event handler. For instance, typing in the input field will only trigger the `on_input_change`, not the `on_selection_change`.
Each interaction done in the ComboBox will only trigger its associated event handler. For instance, typing in the input field will only trigger the `on_input_change`, not the `on_change`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true for typing, but it contradicts earlier docs when user selects a value, that does actually fire on_input_change.

Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Looking better. Looks like you may have accidentally committed a file? Also see the comment about an inaccuracy in ## Control section

@AkshatJawne AkshatJawne requested a review from bmingles August 14, 2024 20:16
bmingles
bmingles previously approved these changes Aug 15, 2024
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/combo_box.md Outdated Show resolved Hide resolved
@AkshatJawne AkshatJawne requested a review from dsmmcken August 19, 2024 13:42
dsmmcken
dsmmcken previously approved these changes Aug 19, 2024
@AkshatJawne AkshatJawne merged commit 563504c into deephaven:main Aug 19, 2024
16 checks passed
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.

docs: ui.combo_box
5 participants