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

Fix gallery style #4576

Merged
merged 9 commits into from Jun 20, 2023
Merged

Fix gallery style #4576

merged 9 commits into from Jun 20, 2023

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Jun 20, 2023

Description

Fixes: #4542

@hannahblair - started working on this before you assigned yourself. Should have assigned myself. Sorry!

@aliabid94 I think this is just an oversight from #4374 ?

Repro:

Deployed this pr to a duplicate of the original repro space (https://huggingface.co/spaces/freddyaboulton/gallery-gradio-3-35-2)

You can clone this locally and build this PR to test locally.

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests & Changelog

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

  3. Unless the pull request is labeled with the "no-changelog-update" label by a maintainer of the repo, all pull requests must update the changelog located in CHANGELOG.md:

Please add a brief summary of the change to the Upcoming Release section of the CHANGELOG.md file and include
a link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by [@myusername](link-to-your-github-profile) in [PR 11111](https://github.com/gradio-app/gradio/pull/11111)".

@hannahblair
Copy link
Collaborator

No worries!

@gradio-pr-bot
Copy link
Contributor

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

@freddyaboulton freddyaboulton marked this pull request as ready for review June 20, 2023 17:58
@abidlabs
Copy link
Member

@freddyaboulton am I missing something or are the preview buttons not being shown anymore at the bottom of the Gallery?

code:

import pathlib

import gradio as gr

paths = [
    path.as_posix() for path in sorted(pathlib.Path('.').glob('c*.jpg'))
]

with gr.Blocks() as demo:
    gallery = gr.Gallery(value=paths)
demo.queue().launch()
image

@freddyaboulton
Copy link
Collaborator Author

@abidlabs - the preview buttons only show up if the image is clicked right? On the docs of the latest release:

image

@abidlabs
Copy link
Member

Ah my bad! I'm still getting weird behavior when clicking on the images to get to the preview:

Recording 2023-06-20 at 14 16 07

Also in an embedded setting (like Jupyter notebook, or Spaces I assume), I'm not able to click on the images at all to get to the preview (nothing happens when you click on them), which I was confused in the first place

@freddyaboulton
Copy link
Collaborator Author

freddyaboulton commented Jun 20, 2023

@abidlabs I just pushed up a fix for the first issue you noted (you can test fix here) - that's reproducible on main and not introduced in this pr. See https://huggingface.co/spaces/hysts-debug/gallery-gradio-3-34-0

Let me look at the jupyter notebook issue now! Thanks for flagging

<div
on:keydown={on_keydown}
class="preview"
class:fixed-height={height !== "auto"}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got rid of fixed-height so that the preview would cover the block and not look like this:

image

class:pt-6={show_label}
>
{#each _value as [image, caption], i}
<button
class="thumbnail-item thumbnail-lg"
class:selected={selected_image === i}
on:click={() => (selected_image = can_zoom ? i : selected_image)}
on:click={() => (selected_image = i)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got rid of can_zoom to fix the gallery click not working on jupyter that @abidlabs noticed. I think the zoom works well on small screens but it's possible I misunderstood the original motivation for this variable

small_gallery_screen

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

LGTM @freddyaboulton. I tried a lot of different parameters and the issues seem to have been resolved, thanks!

Let's write some tests for Gallery later this week.

@abidlabs
Copy link
Member

Even the Gallery from @pngwn's original demo renders fine:

image

@@ -229,15 +222,14 @@
>
<div
class="grid-container"
style="{grid_cols_style} {grid_rows_style}"
style:object_fit
style="{grid_cols_style} {grid_rows_style} --object-fit: {object_fit}; height: {height}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is height set here? it's already set in the parent Block in js/app/src/components/Gallery/Gallery.svelte

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need here otherwise the parameter is not respected in the actual gallery size:

need_height_css

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

LGTM!

@freddyaboulton
Copy link
Collaborator Author

Thanks for the reviews everyone!!

@freddyaboulton freddyaboulton merged commit 8c001ca into main Jun 20, 2023
8 checks passed
@freddyaboulton freddyaboulton deleted the 4542-fix-gallery-style branch June 20, 2023 21:57
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.

Some parameters of gr.Gallery doesn't seem to work with gradio==3.35.2
5 participants