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

Improve gr.Code #9450

Open
wants to merge 14 commits into
base: 5.0-dev
Choose a base branch
from
Open

Improve gr.Code #9450

wants to merge 14 commits into from

Conversation

hannahblair
Copy link
Collaborator

@hannahblair hannahblair commented Sep 26, 2024

Description

  1. Adds a touch of spacing in the code gutter
  2. Ensures we use the min_width param which wasn't being applied to the frontend
  3. Fixes the download and copy icon effect on click
  4. Adds line wrapping to the code editor to prevent having to scroll horizontally to edit non-visible code
  5. Sets max_lines to None by default (breaking change). This means that by default the Code component will fill the container and expand with the content just as the other components do. Setting max_lines to a number will still fix the height.
base new
1 image image
2 image image
3 Kapture 2024-09-26 at 14 32 30 Kapture 2024-09-26 at 14 29 30
4 Screenshot 2024-09-27 at 18 42 20 Screenshot 2024-09-27 at 18 38 01

Closes: #9414
Closes: #6041

🎯 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

  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

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Sep 26, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-pypi-previews.s3.amazonaws.com/f7cf98c86638ef0fd1ee67da686ab13f0b6191e4/gradio-4.44.1-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@f7cf98c86638ef0fd1ee67da686ab13f0b6191e4#subdirectory=client/python"

Install Gradio JS Client from this PR

npm install https://gradio-npm-previews.s3.amazonaws.com/f7cf98c86638ef0fd1ee67da686ab13f0b6191e4/gradio-client-1.6.0-beta.3.tgz

Use Lite from this PR

<script type="module" src="https://gradio-lite-previews.s3.amazonaws.com/f7cf98c86638ef0fd1ee67da686ab13f0b6191e4/dist/lite.js""></script>

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Sep 26, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/code minor
@gradio/icons minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Improve gr.Code

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.

@hannahblair
Copy link
Collaborator Author

I initially amended the height behaviour to auto fill the container, but realised that it would affect the max_lines functionality. The default max_lines value is 20 and renders like this by default:

Screenshot 2024-09-26 at 16 25 00

I'm wondering if some users will want the component just to auto fill the container (thus unset the height):

Screenshot 2024-09-26 at 16 28 10

@aliabid94
Copy link
Collaborator

I'm wondering if some users will want the component just to auto fill the container (thus unset the height):

I think this looks better and also is consistent with all other Blocks that fill to match the height of neighboring containers

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, thanks @hannahblair !

@aliabid94
Copy link
Collaborator

aliabid94 commented Sep 26, 2024

Another thing that'd be nice is if I click in the code component but below the text, the text cursor appears at the end so I can start typing. Referring to this red dead zone:

Screenshot 2024-09-26 at 3 49 00 PM

Because otherwise, when the component is empty, there's only one line to click on which is a narrow space to start typing.

Copy link
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.

Thanks for taking care of this @hannahblair!

I agree with @aliabid94's comments but they aren't blocking.

@hannahblair
Copy link
Collaborator Author

hannahblair commented Sep 27, 2024

Yeah, I agree that the autofill looks better. Perhaps the best way of doing this is allowing max_lines to be set to None to denote that the height is unimportant (right now it only accepts a number) to determine whether height should be fit-content. Something like height={ max_lines ? 'fit-content' : 'auto'}. I'll try implementing this but if anyone has a better approach lmk

edit: actually maybe max_lines should default to None, so the autofill behaviour is the default

@hannahblair
Copy link
Collaborator Author

hannahblair commented Sep 27, 2024

Another thing that'd be nice is if I click in the code component but below the text, the text cursor appears at the end so I can start typing. Referring to this red dead zone:

@aliabid94 I've addressed this by actually introducing line wrapping into the code editor which I think makes it much more usable and you can see the cursor location on focus. Scrolling horizontally feels a bit clunky IMO. Let me know what you think!

Screenshot 2024-09-27 at 18 38 01

@aliabid94
Copy link
Collaborator

aliabid94 commented Sep 27, 2024

nice, thanks for implementing! Looks great! my only concern now is that as I keep typing more lines, the editor just expands indefinitely, e.g. here

Screenshot 2024-09-27 at 11 42 18 AM
import gradio as gr

with gr.Blocks() as demo:
    with gr.Row():
        img = gr.Image(label="Name")
        code = gr.Code()
        ocode = gr.Code()
    btn = gr.Button("Run")
    
    btn.click(lambda x:x, code, ocode)

if __name__ == "__main__":
    demo.launch()

Can we respect the default height of Blocks, and configure via height= kwarg, which is independent of max_lines?

@pngwn
Copy link
Member

pngwn commented Sep 27, 2024

I've addressed this by actually introducing line wrapping into the code editor which I think makes it much more usable

I really think this should be an option if we add it. Line wrapping is not standard for code editors and (imo) is a worse UX.

@hannahblair
Copy link
Collaborator Author

hannahblair commented Oct 1, 2024

@aliabid94 the expanding with the content behaviour is how other components behave, like gr.Textbox, gr.Image, gr.FileUploader etc - we don't have a default height for Block.svelte(Correct me if I'm wrong). We could set an arbitrary max_height of something like var(-size-20), but it feels like that should be up to the user.

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.

4 participants