-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(text-area): add support for minLength
#10970
base: dev
Are you sure you want to change the base?
feat(text-area): add support for minLength
#10970
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great @Elijbet! 🌮
Just a few minor suggestions.
packages/calcite-components/src/components/text-area/assets/t9n/messages.json
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/text-area/resources.ts
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/text-area/text-area.tsx
Outdated
Show resolved
Hide resolved
@@ -1,4 +1,5 @@ | |||
{ | |||
"invalid": "Invalid", | |||
"tooLong": "The current character length is {currentLength}, which exceeds the maximum character length of {maxLength}." | |||
"tooLong": "The current character length is {currentLength}, which exceeds the maximum character length of {maxLength}.", | |||
"tooShort": "The current character length is {currentLength}, which is less than the minimum character length of {minLength}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geospatialem could you review the string wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, @Elijbet - wording LGTM! 🚀
minlength
to text-area
minLength
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few comments, but this looks great, @Elijbet!
Can you also add a screenshot test? We can tweak the Exceeding Max Length Test Only
story to include both min and max tests (along with proper rename).
ariaLabel={getLabelText(this)} | ||
autofocus={this.el.autofocus} | ||
class={{ | ||
[CSS.textArea]: true, | ||
[CSS.readOnly]: this.readOnly, | ||
[CSS.textAreaInvalid]: this.isCharacterLimitExceeded(), | ||
[CSS.textAreaInvalid]: | ||
this.isCharacterOverMaxLimit() || this.isCharacterUnderMinLimit(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's DRY this up by storing and reusing the results of isCharacterOverMaxLimit()
and isCharacterUnderMinLimit()
within this render method.
packages/calcite-components/src/components/text-area/text-area.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/text-area/text-area.e2e.ts
Outdated
Show resolved
Hide resolved
… replacing long and short messages
Related Issue: #8626
Summary
Add support for
minlength
totext-area
.