gradio-app / gradio

Build and share delightful machine learning apps, all in Python. 🌟 Star to support our work!
http://www.gradio.app
Apache License 2.0
29.84k stars 2.22k forks source link

Allows setting a height to `gr.File` and improves the UI of the component #5221

Closed abidlabs closed 9 months ago

abidlabs commented 9 months ago

This PR:

image

As a side note, it seemed like I had to add the height prop to a lot of different places in order to get this to work. Maybe I'm doing something wrong?

Fixes: #5131

gradio-pr-bot commented 9 months ago

🪼 branch checks and previews

• Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
Visual tests all good! Build review
:unicorn: Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/03089aa0bba55adf733cda2a382d574de4a38659/gradio-3.40.1-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@03089aa0bba55adf733cda2a382d574de4a38659#subdirectory=client/python"
gradio-pr-bot commented 9 months ago

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/file minor
gradio minor

With the following changelog entry.

Allows setting a height to gr.File and improves the UI of the component

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](https://github.com/gradio-app/gradio/edit/file-height/.changeset/lovely-bikes-thank.md).
pngwn commented 9 months ago

Passing that prop down a few levels isn't really unexpected, there are alternative approaches we can take if it gets annoying though.We will be able to remove one layer when #5215 is merged.

dawoodkhan82 commented 9 months ago

Screenshot 2023-08-15 at 2 19 08 PM

Looks like we need to add padding for the "x" button. This is when I set height to 200px

dawoodkhan82 commented 9 months ago

Also how do we feel about adding a drop shadow when scrolling is enabled to make it clear that the component can be scrolled.

dawoodkhan82 commented 9 months ago

@abidlabs Few nits, but otherwise Looks good to me! Removing "download" looks much cleaner. Thanks for adding this.

aliabid94 commented 9 months ago

Block.svelte takes a height attribute - see Image component or Chatbot component for examples. Shouldn't have to add it everywhere else.

abidlabs commented 9 months ago

Looks like we need to add padding for the "x" button. This is when I set height to 200px

Will fix, thanks

Also how do we feel about adding a drop shadow when scrolling is enabled to make it clear that the component can be scrolled.

I can take a look at this -- do we have this in any other components?

abidlabs commented 9 months ago

@aliabid94 I couldn't get the Blocks height to work if the the FilePreview component was being used. Plus I think we'll need access to the height parameter inside the FilePreview component to address #5247 so I've left it as is for now.

@dawoodkhan82 I like the suggestion of adding a drop shadow effect -- this feedback applies to several components, so I've moved it to a separate issue: #5247.

Everything else should be addressed!

Let me know if you have any additional feedback @dawoodkhan82 @hannahblair @aliabid94. Otherwise I'll merge in a bit.

abidlabs commented 9 months ago

This branch is starting to act super weird... will close and reopen 😅

Edit: okay all the extraneous changes are gone.

C0untFloyd commented 9 months ago

Thanks for implementing this, however can the removal of the download link be optional? Because I'm using the file component selection as filename input for an image component. Clicking on it with the fix would immediately start a download now, wouldn't it?

abidlabs commented 9 months ago

Thanks for implementing this, however can the removal of the download link be optional? Because I'm using the file component selection as filename input for an image component. Clicking on it with the fix would immediately start a download now, wouldn't it?

Very good point @C0untFloyd I'll redesign it bit

abidlabs commented 9 months ago

Redesigned slightly based on the point that @C0untFloyd brought up.

image

Here's a demo to test both the select feature and downloading links:

import gradio as gr

def s(i, x: gr.SelectData):
    return x.index

with gr.Blocks() as demo:
    gr.File()
    f = gr.File(["cheetah.jpg", "test.mp3"])
    t = gr.Textbox()
    f.select(s, f, t)

demo.launch()

Have merged in #5253, everything seems be working now!

dawoodkhan82 commented 9 months ago

Nice, I like the new download button design! Also tested and everything pretty much works great. Just a few UI nits:

(1) Setting the height to something really small (50px) kinda breaks the UI. Maybe we have a min-height?

Screenshot 2023-08-17 at 4 47 59 PM

(2) On a small screen size, the download icon gets pushed to a new line and looks awkward.

Screenshot 2023-08-17 at 4 46 08 PM

(3) Hard to tell which download link is associated with which file, since there's so much white space in between. Not necessarily an issue with your changes, but maybe we open a new issue for this?

Screenshot 2023-08-17 at 4 46 53 PM
abidlabs commented 9 months ago

Thanks @dawoodkhan82 for the re-review!

(1) I think its fine to leave this to the whims of the developer. The same behavior is true when setting the height of the Image component to a very small value, for example, and it hasn't posed any issues.

(2) Ah yeah let me fix this

(3) Yeah I noticed this as well. Let me play around with something.

abidlabs commented 9 months ago

Ok I fixed both of those issues @dawoodkhan82! For (3), I took a leaf from the gr.Dataframe and added an alternating background. See mockups below:

image

Dark mode:

image
dawoodkhan82 commented 9 months ago

LGTM very clean!

dawoodkhan82 commented 9 months ago

(1) I think its fine to leave this to the whims of the developer. The same behavior is true when setting the height of the Image component to a very small value, for example, and it hasn't posed any issues.

I feel like we should rethink this. We should always try and make it impossible (or at least very hard) for a dev to create a broken or ugly demo.

abidlabs commented 9 months ago

I feel like we should rethink this. We should always try and make it impossible (or at least very hard) for a dev to create a broken or ugly demo.

That's fair -- maybe we can tackle this as part of https://github.com/gradio-app/gradio/issues/5247, since both of these issues govern components whose height can be set

abidlabs commented 9 months ago

Thanks for the detailed review @dawoodkhan82! Pretty happy with where we ended up

jkyndir commented 5 months ago

i'm using gradio 4.11.0 in my project. but the height setting doesn't seem to take effect on gr.File at all?

any help would be appreciated

HuntZhaozq commented 5 months ago

i'm using gradio 4.11.0 in my project. but the height setting doesn't seem to take effect on gr.File at all?

any help would be appreciated

I meet the same problem. How to solve this ? @abidlabs

vernikouskaya commented 3 months ago

Hi, I constantly update gradio, but since version 4.15 I used for the first time there is no scroller component appearing in gr.Files if the "height" parameter is set. This is my code line: files_input = gr.Files(file_types=['image'], type='filepath', height=256, interactive=True) I would really appreciate any help!

abidlabs commented 3 months ago

Hi @vernikouskaya I can reproduce the problem. Would you be able to create a new issue for this?