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

gr.Dropdown now has correct behavior in static mode as well as when an option is selected #5062

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Aug 1, 2023

2 more fixes to gr.Dropdown():

  1. In static mode, the gr.Dropdown(multiselect=True) still had the X icons, and clicking on an option would remove it. This behavior has been disabled in static mode. Test with:
import gradio as gr

with gr.Blocks() as gr_training_zoo:
    d2 = gr.Dropdown(
        ["ran", "swam", "ate", "slept"],
        value=["swam"],
        multiselect=True,
        label="Initialized",
        info="Only value removal allowed, no adding",
    )

gr_training_zoo.launch()

Also we had never really documented how Gradio determines the interactivity of a component if interactive was not specified and the component was neither an input nor an output. I've added that to the relevant Guide. Together, this closes: #4280.

  1. Clicking on a dropdown option gr.Dropdown(multiselect=False) would select it, yet it would keep the focus on the Dropdown and in particular would keep the cursor typing. This led to an interesting bug described in Dropdown pops up if focus is lost during change. #4269. I've changed the behavior so that clicking on a choice actually unfocuses from the dropdown. This is a more satisfying UX, more similar to the native select element's UX, and closes: Dropdown pops up if focus is lost during change. #4269.

Also added a few gr.Dropdown stories to prevent regression of the former issue.

@vercel
Copy link

vercel bot commented Aug 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview Aug 1, 2023 9:56pm

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 1, 2023

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/form patch
gradio patch
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

gr.Dropdown now has correct behavior in static mode as well as when an option is selected

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 1, 2023

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-5062-all-demos


You can install the changes in this PR by running:

pip install https://gradio-builds.s3.amazonaws.com/8745dac99c5523561c7cd3296805500538c611c0/gradio-3.39.0-py3-none-any.whl

@abidlabs abidlabs marked this pull request as ready for review August 1, 2023 21:55
@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 1, 2023

🎉 Chromatic build completed!

There are 0 visual changes to review.
There are 0 failed tests to fix.

Comment on lines -112 to -114
} else {
filterInput.blur();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Unless I'm missing something, these lines can never be reached @dawoodkhan82?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think your cleanup of this makes sense. But I think these lines should've been hit. How else was the input being blurred? But I also didn't go back and test on main, so not sure.

Copy link
Collaborator

@dawoodkhan82 dawoodkhan82 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. I also tested kitchen-sink and the dropdown works fine. One nit for the static dropdown would be to change the cursor from pointer to default when hovering over the token.

Comment on lines -112 to -114
} else {
filterInput.blur();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think your cleanup of this makes sense. But I think these lines should've been hit. How else was the input being blurred? But I also didn't go back and test on main, so not sure.

@abidlabs
Copy link
Member Author

abidlabs commented Aug 2, 2023

Thanks for the review @dawoodkhan82!

One nit for the static dropdown would be to change the cursor from pointer to default when hovering over the token.

I'll fix that!

But I think these lines should've been hit. How else was the input being blurred?

What do you mean? Why should the input be blurred in the focus method? Maybe I'm missing something

This was never hit because showOptions would always be false before you focused. In the focus method, we invert it, so it would become true, so this logic was never hit.

@dawoodkhan82
Copy link
Collaborator

This was never hit because showOptions would always be false before you focused. In the focus method, we invert it, so it would become true, so this logic was never hit.

Ah yes I see what you mean. Yes we're good to remove it then!

@abidlabs
Copy link
Member Author

abidlabs commented Aug 2, 2023

Sounds good, thanks for the confirmation.

One nit for the static dropdown would be to change the cursor from pointer to default when hovering over the token.

I was rethinking this, and I think its better to leave as is because clicking while disabled still triggers the .select() event trigger (similar to the other components that support .select())

@abidlabs abidlabs merged commit 7d89716 into main Aug 2, 2023
10 checks passed
@abidlabs abidlabs deleted the static-dropdown branch August 2, 2023 16:32
@pngwn pngwn mentioned this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants