Skip to content

Reworking layout styling#2291

Merged
aliabid94 merged 29 commits into
mainfrom
fix_layout
Oct 3, 2022
Merged

Reworking layout styling#2291
aliabid94 merged 29 commits into
mainfrom
fix_layout

Conversation

@aliabid94
Copy link
Copy Markdown
Contributor

This is a proposal on reworking how adjacent components can be styled to be "grouped" together, such that there is no gap between them and only the components at the ends of the group are rounded to create the visual effect of being a single group. This is currently done as such:

  • Create a parent gr.Box to create the rounded style,
  • Then create a gr.Group inside to remove spacing between elements.
  • Then for each nested component, remove the margins on the correct sides
  • Then target the individual border radiuses to such that internal components have no rounding, and components at the ends only round the sides at the end.

This is quite complicated, and not very mobile friendly. For example, imagine two components A and B placed horizontally. We may remove the right margin of A and round the left border of A (and the inverse for B). But if the screen shrinks such that A and B must be placed vertically, all margins and rounding will be incorrect.

A more effective version of this logic is already done with the gr.Form component (which automatically wraps all adjacent "form" components). gr.Form removes the gap between all adjacent components and provides a rounded border to combine them all.

What I propose is similar CSS styling logic used in a new variant for gr.Row() and gr.Column(). A gr.Row(variant="group") or gr.Column(variant="group") will automatically combine all child components into a single "group", removing gaps and setting borders as necessary.

BEFORE:

with gr.Blocks() as demo:
    with gr.Group():
        with gr.Box():
            with gr.Row().style(mobile_collapse=False, equal_height=True):
                text = gr.Textbox().style(
                    border=(True, False, True, True),
                    rounded=(True, False, False, True),
                    container=False,
                )
                btn = gr.Button("Go").style(
                    margin=False,
                    rounded=(False, True, True, False),
                    full_width=False
                )

AFTER:

with gr.Blocks() as demo:
    with gr.Row(variant="group"):
      text = gr.Textbox().style(container=False)
      btn = gr.Button("Go").style(full_width=False)

Along with this change, I would like to deprecate:

  • gr.Group
  • gr.Box
  • setting rounded and margin styles per corner (I would also consider removing them entirely)

I believe this new variant proposal is in the spirit of "the user should not be able to create a bad looking demo with our style API".

@github-actions
Copy link
Copy Markdown
Contributor

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

@pngwn
Copy link
Copy Markdown
Member

pngwn commented Sep 19, 2022

With this change, is there anything that is currently possible but would not be possible if we remove Box and Group?

I think the changes look good, just not sure if deprecating Box and Group will cause issues.

@aliabid94 aliabid94 marked this pull request as ready for review September 19, 2022 22:33
@aliabid94
Copy link
Copy Markdown
Contributor Author

So I removed the ability to set margins, rounding and borders because I don't think it made sense - these should be automatically be rendered based on the adjacent components, not on a per-component basis. We'll have to discuss what exactly we want users to be be able to control per-component, and what they can control via theming.

See demo/fake_gan for the gr.Row(variant="group")

Copy link
Copy Markdown
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

This looks goo to me and I agree about settings margins, borders and border-radiuses, that no longer makes sense since it is handled by the layout system. I think should be settable on a global level but not sure what makes sense on a per component basis. I think probably none of them (expect perhaps borders themselves).

Only thing to be aware of is that this is a pretty significant breaking change: people using sone the old style args will no longer have the same layout and it will look bad in weird ways. And people using layout defaults will have apps that look different. I think in most cases this will be an improvement and there shouldn't be too many negative effects (as I think usage of style props was low), so I think this is okay given the improvements it brings. We just need to be aware of it from a support PoV.

@abidlabs
Copy link
Copy Markdown
Member

abidlabs commented Sep 20, 2022

I released a beta version (3.4b1) to try this out and see if there were any breaking changes. I recreated a few Spaces and for the most part, everything seems to be working fine.

However, when I was recreating the Stable Diffusion Space, I noticed the button and textbox take equal width when inside a gr.Row(variant="group"), like this:

image

Previously (in gradio<3.2), the button only took up as much space as it needed, which was a better UI:

image

Is there a way to get this UI with these new layout changes? Here's my code: https://colab.research.google.com/drive/1vg4aV2_x3kf6uOitslc6Vsi2kqxeI7vV?usp=sharing

In particular, when testing for backwards compatibility, can we make sure that the web uis for Stable Diffusion do not break since many people have cloned those and are running on colab, which means they will automatically use the latest version? (In fact, we should mock up a couple of the popular SD web UIs and add them to the Spaces previewer to be 100% sure)

@aliabid94
Copy link
Copy Markdown
Contributor Author

you have to set gr.Button().style(full_width=False). This was true in the old demo as well.

Previously (in gradio<3.2), the button only took up as much space as it needed, which was a better UI:

@abidlabs
Copy link
Copy Markdown
Member

abidlabs commented Sep 28, 2022

Btw @aliabid94 does this close #2152? If so, we should add that to the PR description

@abidlabs
Copy link
Copy Markdown
Member

abidlabs commented Oct 3, 2022

Awesome @aliabid94! Tested a bunch of demos and they all LGTM

@aliabid94 aliabid94 merged commit 1f5efa7 into main Oct 3, 2022
@aliabid94 aliabid94 deleted the fix_layout branch October 3, 2022 23:01
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.

3 participants