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
Fix gr.Group, container=False #4916
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🎉 The demo notebooks match the run.py files! 🎉 |
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4916-all-demos You can install the changes in this PR by running: pip install https://gradio-builds.s3.amazonaws.com/c409cd5c730ef320b00fbef249fcc168f2d3e51d/gradio-3.36.1-py3-none-any.whl |
@@ -35,6 +35,7 @@ | |||
style:overflow={allow_overflow ? "visible" : "hidden"} | |||
style:flex-grow={scale} | |||
style:min-width={`calc(min(${min_width}px, 100%))`} | |||
style:border-width="var(--block-border-width)" |
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.
no idea why I needed to do this but was not picking up the css variable otherwise
gradio/components/dropdown.py
Outdated
@@ -101,6 +101,13 @@ def __init__( | |||
raise ValueError( | |||
"Custom values are not supported when `multiselect` is True." | |||
) | |||
if not container: | |||
if show_label: | |||
warnings.warn("show_label has no effect when container is False.") |
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.
We should use utils.warn_deprecation
here! And in other places were issuing deprecation warnings.
@@ -42,7 +42,6 @@ | |||
export let label: string; | |||
export let show_label: boolean; | |||
export let colors: Array<string>; | |||
export let container: boolean = false; |
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.
In some of these files we delete the container
prop (like in this one) but in others we keep the prop but don't pass it to the component (like video). I think we should remove the prop entirely from all the front-end components (except the form ones you laid out).
BTW, why only Dropdown, Number, Textbox and not all the form components? Just wondering
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.
Restored container everywhere.
Textbox, Dropdown, and Number have slightly container behavour in that instead on the block border disappearing, the input border element disappears. Makes it easier to play with gr.Group.
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.
Made some minor comments but otherwise this looks good to me! I would add the demo you made in the PR description to all_demos so we can keep track of this behavior before release (in lieu of app-level visual testing). Might be good to add some visual stories for the form components with and without containers too.
@aliabid94 I was testing this branch in conjunction with the with gr.Blocks() as demo:
with gr.Group():
gr.Chatbot()
with gr.Row():
with gr.Column(scale=10):
gr.Textbox(container=False)
with gr.Column(scale=1, min_width=0):
gr.Button()
demo.launch() Which produces: Notice the double border on the bottom This problem goes away if I remove the with gr.Blocks() as demo:
with gr.Group():
gr.Chatbot()
with gr.Row():
gr.Textbox(container=False, scale=10)
gr.Button(scale=1, min_width=0)
demo.launch()
Edit: I removed |
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.
The implementation looks good to me in general but I do have a question.
What is the rationale for removing container
on some of these components? I understand it for components that fill their container completely (like Image
). But components like Label
, Checkbox
, Plot
do not fill their container and users might want a simple way to remove both the container and the padding that doesn't require some pretty specific CSS.
In this PR, it seems like container=False
has only been considered in so much as it serves Group
but container=False
has broader application than that.
The other thing is that this is a massive breaking change. We need to add the container
kwarg back for everything that had them previously with similar functionality until 4.0.
Yes we'll need to do that, and put a deprecation warning like @freddyaboulton suggested above |
🎉 Chromatic build completed! There are 24 visual changes to review. |
I restored container kwarg for all components. Updated demo code at top that should showcase everything that has changed. Ready for re-review |
Yeah this one's quite tricky, I struggled with it for a while. Would you consider it a blocker? fwiw before this PR, Groups also imposed a Column layout, they did not inherit flex-direction. This will have a follow up PR later where I replace Form entirely with Group in 4.0, was thinking of handling this case then too. |
Nope not a blocker. Just something I noticed in testing. In fact, if it was the previous behavior, we should document this and keep as is to prevent apps from breaking when they upgrade I'll do some more testing of this PR to see if I notice any breaking changes! |
@@ -133,7 +133,7 @@ def __init__( | |||
value: Any = None, | |||
label: str | None = None, | |||
info: str | None = None, | |||
show_label: bool = True, | |||
show_label: bool | None = None, |
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.
Would be good to update the docstring to reflect that show_label=None
falls back to the container
param
Tested a bunch of demos, no issues from my end. Would be good to get another pair of eyes on this, @pngwn @freddyaboulton |
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 @aliabid94 ! Went through the demos again and did not notice any unintended regressions.
The rendering for gr.Group and container=False had been wack for a while. Fixed in this PR. Only Textbox, Number, and Dropdown can set container=False.
In another PR, we can remove Forms entirely and use Groups to bind Form elements, since they render the same. Can do in 4.0 to deprecate Forms.
Example code: