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

enhancement: add max-length, disabled to text and text_area #100

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

mscolnick
Copy link
Contributor

@mscolnick mscolnick commented Sep 12, 2023

image
image

@vercel
Copy link

vercel bot commented Sep 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 11:11pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 11:11pm

@@ -326,6 +326,9 @@ class text(UIElement[str, str]):
- `placeholder`: placeholder text to display when the text area is empty
- `kind`: input kind, one of `"text"`, `"password"`, `"email"`, or `"url"`
defaults to `"text"`
- `max_length`: maximum length of input
- `min_length`: minimum length of input
Copy link
Contributor

Choose a reason for hiding this comment

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

When I test, min_length has no (visible) effect on the frontend, and no effect on Python either. Values less than the min length are still sent to Python, with or without forms. Testing on Chrome.

If someone makes a text input with min length, say 8, but types in only 3 characters, what will text.value be? I guess it might be the empty string, but that's kind of awkward because "" has length 0, ha ...

Maybe min_length doesn't make as much sense as max_length, and can just be validated in Python by the developer? In addition to UX, max_length is helpful for the developer to prevent users from OOM-ing the kernel by typing in an absurdly long string. Developers will have to handle min_length validation in Python, but at least strings that are too short won't have any performance issues.

Given these considerations, my vote is to just not have a min_length option.

@@ -57,15 +63,26 @@ const TextComponent = (props: TextComponentProps) => {
url: <GlobeIcon size={16} />,
};

const endAdornment = props.maxLength ? (
<span className="text-muted-foreground text-xs font-medium">
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to a more visible color (like an amber or red) when the max length is hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i feel that is usually if you go over, instead of exactly, and this will just stop at the max_length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe amber isn't as "woah something is wrong"

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fine to keep as is for now. The only reason I suggested it is in case a user doesn't see the character count get confused why they can't continue typing

@mscolnick mscolnick changed the title enhancement: add min-length, max-length, disabled to text and text_area enhancement: add max-length, disabled to text and text_area Sep 12, 2023
@@ -57,15 +63,26 @@ const TextComponent = (props: TextComponentProps) => {
url: <GlobeIcon size={16} />,
};

const endAdornment = props.maxLength ? (
<span className="text-muted-foreground text-xs font-medium">
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fine to keep as is for now. The only reason I suggested it is in case a user doesn't see the character count get confused why they can't continue typing

@mscolnick mscolnick merged commit e74aeb7 into main Sep 12, 2023
11 checks passed
@mscolnick mscolnick deleted the ms/add-min-max-length branch September 12, 2023 23:34
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants