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

Add combobox widget #2390

Merged
merged 8 commits into from Jun 28, 2019

Conversation

@vidartf
Copy link
Member

commented Apr 24, 2019

Based on adding a <datalist> tag to the <input> of the Text widget.

Fixes #1871.
Fixes #692.

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Commands to try in a notebook:

from ipywidgets import Combobox

Combobox(options=['Chocholate', 'Coconut', 'Vanilla'])

Combobox(options=['Chocholate', 'Coconut', 'Vanilla'], ensure_option=True)
@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Could we instead simply add a datalist attribute to the existing Text input?

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Sure, but that would change the logic some (options would default to None). Would you still always add the datalist tag, even if options is None? Or add/remove this tag as needed?

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

And just for reference: What are the arguments for/against adding to Text vs keeping is separate?

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I was thinking it would be easier for us to maintain and easier for the user (one widget instead of two). However, thinking out continuous_update - it should probably default to false for combobox, but already defaults to true for Text, so maybe that's an argument there for making it a subclass like you have.

Can you change the default for continuous_update? Especially if we're selecting from some options, we should only send a change if we're done selecting, rather than every keystroke.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Can you change the default for continuous_update? Especially if we're selecting from some options, we should only send a change if we're done selecting, rather than every keystroke.

And especially if one option is a substring of another. Continuous update true guarantees that we'll send the substring before the intended option if we are typing it out, which is probably not what is intended.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@SylvainCorlay - +1 for getting something like this in (however it ends up in) for 7.5.

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@SylvainCorlay - +1 for getting something like this in (however it ends up in) for 7.5.

Grumbles 😄

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@jasongrout / @SylvainCorlay This is already available as a separate package, so no rush from my side to get it in.

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@jasongrout Regarding continuous_update, there are two main modes for a combobox:

  • Force the user to select one of the options. Here, continuous updates does not make sense. That is why setting ensure_option to true implies that continuous_update is ignored until a valid value is selected.
  • Options are meant as hints for auto-completion, but free-form input is also valid. Here, continuous_update should behave as for Text (IMHO).
@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

And especially if one option is a substring of another. Continuous update true guarantees that we'll send the substring before the intended option if we are typing it out, which is probably not what is intended.

Hmm, this is a decent counter-example to what I wrote, but makes sense. It is also what I had originally intended (see docstring where it says that enure_option=True implies continuous_update=False).

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Options are meant as hints for auto-completion, but free-form input is also valid. Here, continuous_update should behave as for Text (IMHO).

I think changing the user interaction behavior like this is confusing. I'd rather have continuous_update be explicitly set, rather than changing the default based on another option.

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@jasongrout But I think the other option would still be needed, and that options will still be in conflict with continuous_update no matter what the default are.

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Use cases:

  1. Alice wants a text box with some helpful suggestions (previous entries), but wants to accept new values as well. Also she ...
    1. ... does not need continuous updates, and also does not care if they happen or not, as she has a Button to indicate that she is done (has other input as well).
    2. ... only wants an update when the user indicates that she is done by completing input.
    3. ... needs continuous updates to perform some secondary action while inputting.
  2. Bob wants to force the user to select one of his options, but prefers a ComboBox over a Select, as he wants the user to be able to quickly search by typing. As such, any partial value updates would break his logic.
@vidartf

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Are there any other use cases that I'm missing?

@vidartf

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

I fixed some performance issues now as well, so now it should be able to handle options in the scale 10k-100k. The worst performance is when changing the options list, due to the large number of DOM-nodes that are added/removed.

@vidartf vidartf force-pushed the vidartf:combo branch from 2950fc2 to 4b331a0 May 17, 2019

@SylvainCorlay SylvainCorlay added this to the 7.5 milestone Jun 4, 2019

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

ping @jasongrout ;)

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

I won't be able to review this before 7.5, but it looks like Sylvain is on a review/merge roll.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@vidartf vidartf force-pushed the vidartf:combo branch from 4b331a0 to 41ed5b9 Jun 27, 2019

@SylvainCorlay SylvainCorlay force-pushed the vidartf:combo branch from 41ed5b9 to 082965c Jun 28, 2019

vidartf and others added some commits Jun 28, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@vidartf I am fine with merging the current state of the code. We probably want to have a docstring instead of a TODO statement for the release though. Do you have a bit of time to add something there?

@SylvainCorlay SylvainCorlay merged commit 656e568 into jupyter-widgets:master Jun 28, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@vidartf vidartf deleted the vidartf:combo branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.