Skip to content

textbox-autoheight#1009

Merged
omerXfaruq merged 10 commits into
mainfrom
textbox-autoheight
Apr 20, 2022
Merged

textbox-autoheight#1009
omerXfaruq merged 10 commits into
mainfrom
textbox-autoheight

Conversation

@omerXfaruq
Copy link
Copy Markdown
Contributor

  • add max-lines to textbox

Backend side of #980

Comment thread gradio/components.py Outdated
default_value: str = "",
*,
lines: int = 1,
max_lines: int = 100,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about we leave max_lines as None by default, which allows the Textbox to get arbitrarily large. And set its type to be Optional[int]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had the same intention, my arbitrarily large number was 100, what is your arbitrarily large number? Frontend will have its own max value, so we can give a default value of arbitrarily large number.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does there need to be a max value set in the frontend? I would set the default for max_lines to be None, meaning that there is no max -- the textbox just keeps getting longer.

Copy link
Copy Markdown
Contributor Author

@omerXfaruq omerXfaruq Apr 19, 2022

Choose a reason for hiding this comment

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

@abidlabs I think we would not let Textbox grow infinitely large, that's what I understood from our discussions and how I feel.
What are your thoughts on this @pngwn?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think allowing the textbox to grow infinitely large is a poor default, we can easily set a default in the frontend though without there being one in the back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about allowing 1 million max_lines if the user prefers. I think we would have a max for that as well right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Btw this design assigns a max value(100) when the user does not specify it, we could increase it if we wish. I don't see a need for passing None to frontend and frontend selecting that max value.

Copy link
Copy Markdown
Member

@abidlabs abidlabs Apr 19, 2022

Choose a reason for hiding this comment

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

Yeah, if we are going to set one in the frontend, we should make it consistent in the backend (and allow a user to override it to be much larger if they want). 100 is fine as a default, I suppose?

Edit: thinking from a UX perspective, I actually feel it should be less than 100. 20?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@farukozderim We dont' need to pass anything to the front end particularly, the absence of any preference can trigger a default. I don't think UX defaults should live in the python library particularly.

20 is a better default. We don't want to a hard maximum because people may want longer 'prose'.

@abidlabs
Copy link
Copy Markdown
Member

Looks good, see my comment above -- note that we will also need to fix a bunch of tests that currently test the config produced by the Textbox

Base automatically changed from blocks-dev to main April 19, 2022 09:24
- add max-lines to textbox
- tweaks on scripts
- fix tests
- fix tests
- fix tests
- convert default max_height from 100 to 20
- convert default max_height from 100 to 20
@omerXfaruq omerXfaruq merged commit 9cd4c31 into main Apr 20, 2022
@omerXfaruq omerXfaruq deleted the textbox-autoheight branch April 20, 2022 11:25
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