feat: add text-area component #80
Conversation
|
✅ Deploy Preview for scouterna-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
scriptcoded
left a comment
scriptcoded
left a comment
There was a problem hiding this comment.
Looking very good! Nice job 👏 I've got some very technical comments on how I believe this should connect with the existing Field component. Other than I've just got some small nits and nice-to-haves.
| <div class="flexbox"> | ||
| <label> | ||
| {this.label} | ||
| </label> | ||
| <textarea | ||
| class="textarea" | ||
| autofocus={this.autofocus} | ||
| disabled={this.disabled} | ||
| form={this.form} | ||
| maxLength={this.maxLength} | ||
| placeholder={this.placeholder} | ||
| readOnly={this.readOnly} | ||
| required={this.required} | ||
| value={this.value} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Nice that you've added a label as well! We've set up a generic Field component that wraps all input type components. I forgot to tell you about that, sorry 😣 I think we should add support for that here as well and remove the label from this component. In other words:
- Remove the
divandlabelso that only thetextareais left. - Implement the following from the
Inputcomponent (reference code):- Add the
scoutInputChangeevent that tells the form field about changes that should cause re-validation. - Add the
scoutBlurevent that tells the form field that the field is ready to be validated. - Add the internal
_fieldIdevent that's used by the form field to connect it to the text area. - Add the internal
ariaIdstate that keeps track of the internal ID used for connecting to form field. - Assign the
ariaIdstate in thecomponentWillLoadhook. - Handle the
onInputevent and potential validation.
- Add the
- See other comment about shadow DOM.
- Finally you might want to add another story in
field.stories.tsxto test it out together, like we did for the input.
| <label> | ||
| {this.label} | ||
| </label> | ||
| <textarea |
There was a problem hiding this comment.
Can we also add the name prop? Just realized we're missing that on all inputs. I'll add it to the others in a separate PR 😊
| height: var(--spacing-24); | ||
| width: var(--spacing-50); |
There was a problem hiding this comment.
I think we should keep the width and height unset so that the consumer can choose how to size the component. What we could do instead is expose the rows attribute as a prop and maybe set it to a higher default, say 3?
| shadow: { | ||
| delegatesFocus: true, | ||
| }, |
There was a problem hiding this comment.
Related to other comment about adding support for the Field component, you'll have to disable the shadow DOM and use scoped styles for it to work:
| shadow: { | |
| delegatesFocus: true, | |
| }, | |
| scoped: true, |
| <div> | ||
| {this.label} | ||
| </div> | ||
| <textarea |
There was a problem hiding this comment.
Can we maybe add the name prop as well? I just realized we don't have it on any of our inputs, so I'll add it to the rest in a separate PR 😊
| height: var(--spacing-24); | ||
| width: var(--spacing-50); |
There was a problem hiding this comment.
I think we should leave out the width and height and let the consumer control this as it will most likely depend on their layout. Maybe instead we can expose the rows attribute as a prop and set the default to something a bit higher, say 3?
No description provided.