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.

Sign up for GitHub

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

nimble-text-area should allow setting width #457

Merged
merged 10 commits into from
Mar 31, 2022
+76 −2
Merged

nimble-text-area should allow setting width #457

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
89ad72c
textarea inherit width
m-akinc Mar 24, 2022
8d888bb
Change files
m-akinc Mar 24, 2022
f255781
Also handle text area height
m-akinc Mar 24, 2022
1dc4a11
Improve sizing behavior and add tests
m-akinc Mar 29, 2022
20eb2ad
Merge branch 'main' into users/makinc/text-area-width
m-akinc Mar 29, 2022
ede6ad1
Change sizing test to use createMatrix
m-akinc Mar 30, 2022
9ee6deb
Merge branch 'main' into users/makinc/text-area-width
m-akinc Mar 30, 2022
899d6d3
Test readability suggestion
m-akinc Mar 31, 2022
75fc776
Merge branch 'users/makinc/text-area-width' of https://github.com/ni/…
m-akinc Mar 31, 2022
3050dbb
Merge branch 'main' into users/makinc/text-area-width
m-akinc Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@ni-nimble-components-3d1e9025-e654-496f-a304-152c399e5016.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "textarea inherit width",
"packageName": "@ni/nimble-components",
"email": "7282195+m-akinc@users.noreply.github.com",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions packages/nimble-components/src/text-area/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const styles = css`
.control {
-webkit-appearance: none;
font: inherit;
width: inherit;
Copy link
Member

@rajsite rajsite Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Setting width to inherit probably doesn't behave as expected. See the following example: https://codepen.io/rajsite/pen/WNdRXdK

A good rule of thumb is to generally avoid configuring properties as inherit that are not configured as inherited by default: https://developer.mozilla.org/en-US/docs/Web/CSS/width#formal_definition

I'd probably stick to what fast does and set the width to 100% for the .control textarea to make sure it is sized to the whatever the nimble-text-area was sized to by the developer: https://github.com/microsoft/fast/blob/6e3de2afdf8c7be7c2be369e8386f114e6e6417e/packages/web-components/fast-components/src/text-area/text-area.styles.ts#L58

My guess is that most folks are more concerned that the width and height are respected on the nimble-text-area than the width and height as defined by the rows and cols attributes.

Edit: If we end up going with the recommendation here it may be worth adding a comment to the css that says we are explicitly setting the width and height on the textarea which overrides the behavior of rows and cols (maybe be worth adding a note in the storybook too for those properties).

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to handle both cases? With something like

.control {
  width: 100%;
}

:host([cols]) .control {
  width: inherit;
}

Copy link
Member

Choose a reason for hiding this comment

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

Seems like that could work 👍 we should include some storybook tests to prevent regressions and test out sizing both ways

We may need to verify the same issues aren't showing up with rows config

Copy link
Contributor Author

@m-akinc m-akinc Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we set width=100% on the textarea element, then setting cols does nothing. To be clear, you're suggesting we remove that attribute from the public API of the text area, right? That makes sense to me, though it's different than the html textarea and the FAST text area.

What do you think we should do about the height? Same? Or leave it controlled by the rows attribute?

Didn't see these responses before I replied. I think that answers my questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Milan notes below, inherit has some issues. One of them being the !=100% multiplicative issue. Another being that if both width and cols are both set, width wins. If we document that, it's fine, but then we don't really need to be special casing :host([cols]) .control anyway, right?

Another similar issue is how to to handle the height. There may or may not be a control label, and this affects how we would want to set the height. I can do height: calc(100% - ${controlLabelFontLineHeight}), but that is only correct if there is a label. If not, it's too short. Maybe for height we do force them to specify rows? Thoughts?

Copy link
Member

@rajsite rajsite Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here is one idea I prototyped: https://github.com/ni/nimble/compare/users/makinc/text-area-width...text-area-width-prototype?expand=1

Layout the label and the text area in a vertical flex column. Tell the label to not flex in size (just sizes to it's content). Tell the textarea to grow (it's the only growing item in the flex layout so it takes the rest of the space).

Without rows or cols specified the flex fills the height and the width 100% fills the width. With cols specified the width is switched to auto and the browser will size the textarea based on the cols. When rows is specified we say don't flex size and the browser will size based on rows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works great. The only case it doesn't handle perfectly is when a width is set on the nimble-text-area along with a cols value. In that case, the auto width on the textarea control ends up taking the width of the containing host rather than using the cols value. Still, that seems a very minor issue.

outline: none;
box-sizing: border-box;
position: relative;
Expand Down