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

nimble-text-area should allow setting width #457

Merged
merged 10 commits into from
Mar 31, 2022
Merged

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Mar 24, 2022

Pull Request

🤨 Rationale

The nimble-text-area cannot be made to stretch to fill the width of its container. Or rather, the contained textarea does not obey the width set on the nimble-text-area.

👩‍💻 Implementation

Setting width to inherit on the textarea (class control) causes it to inherit any width value set on the nimble-text-area element. Note that nimble-text-area also supports controlling the visible width via a cols attribute (number of visible text columns). When the width style is set, this overrides the cols attribute.

🧪 Testing

Tested manually in dev tools.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc m-akinc changed the title Users/makinc/text area width nimble-text-area should allow setting width Mar 24, 2022
@@ -43,6 +43,7 @@ export const styles = css`
.control {
-webkit-appearance: none;
font: inherit;
width: inherit;
Copy link
Member

@rajsite rajsite Mar 24, 2022

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

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

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.

@m-akinc m-akinc requested a review from rajsite March 29, 2022 21:38
@m-akinc m-akinc requested a review from rajsite March 30, 2022 20:16
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

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

This has received sufficient attention from other reviewers, so don't wait on me to submit.

@m-akinc m-akinc enabled auto-merge (squash) March 31, 2022 20:44
@m-akinc m-akinc merged commit 93644b5 into main Mar 31, 2022
@m-akinc m-akinc deleted the users/makinc/text-area-width branch March 31, 2022 20:54
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.

4 participants