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

Add Textarea component #1204

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Add Textarea component #1204

merged 1 commit into from
Aug 29, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Aug 21, 2023

This PR depends on #1196

Add a new Textarea component that renders consistently with Input and Select.

image

You can see how it looks by checking out this branch, running make dev and going to http://localhost:4001/input-textarea

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #1204 (7732f62) into main (e6a1ee8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1204   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          57       58    +1     
  Lines         808      812    +4     
  Branches      313      314    +1     
=======================================
+ Hits          807      811    +4     
  Misses          1        1           
Files Changed Coverage Δ
src/components/input/InputRoot.tsx 87.50% <ø> (ø)
src/components/input/Textarea.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acelaya acelaya requested a review from robertknight August 21, 2023 14:55
@acelaya
Copy link
Contributor Author

acelaya commented Aug 22, 2023

I just noticed, in TypeScript, the type for textarea elements is HTMLTextAreaElement, so I'm thinking this component should be called TextArea, not Textarea 🤔

@acelaya acelaya force-pushed the input-feedback branch 2 times, most recently from d0beae4 to 399e79f Compare August 23, 2023 14:07
@robertknight
Copy link
Member

I just noticed, in TypeScript, the type for textarea elements is HTMLTextAreaElement, so I'm thinking this component should be called TextArea, not Textarea

It depends whether you count "textarea" as one word or two. From a quick Google search, Textarea seems to be more common in other web UI libraries than TextArea. See also "Checkbox" and "Snackbar".

Base automatically changed from input-feedback to main August 24, 2023 09:18
@acelaya
Copy link
Contributor Author

acelaya commented Aug 24, 2023

It depends whether you count "textarea" as one word or two. From a quick Google search, Textarea seems to be more common in other web UI libraries than TextArea. See also "Checkbox" and "Snackbar".

Good point. Textarea it is then.

@acelaya acelaya marked this pull request as ready for review August 24, 2023 09:21
@acelaya acelaya force-pushed the textarea branch 3 times, most recently from 6b8d386 to f80de34 Compare August 28, 2023 10:18
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM, but I suggest we remove the InputGroup example, since that component was designed for single-line inputs and looks weird if you combine it with a Textarea.

/**
* Render a textarea
*/
const Textarea = function Textarea({
Copy link
Member

Choose a reason for hiding this comment

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

Can you remember the reason for the whole const Widget = function Widget(...) { ... }; export default Widget pattern? I think it had something to do with a migration at one point.

I'd like to simplify this code back to export default function Widget(...) { ... } when we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was the reason. We had at one point some cases of const WidgetNext = function Widget() {...} (or the other way around).

We agreed to simplify it after the old components were removed, so nothing should stop us now.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I filed #1215 as a reminder.

</InputGroup>
</div>
</Library.Demo>
</Library.Pattern>
Copy link
Member

Choose a reason for hiding this comment

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

The "Textarea in an InputGroup" example looks weird:

Textarea input group

Since we don't have an immediate use case for this, I would just remove this example. If we do want this in future, we'll need to come up with a different design that doesn't look silly. For example we could add the copy button in a footer for the control or as a standalone button in the lower-right corner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

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.

2 participants