refactor: #136 TextArea - Add CSS resizing #150

kurtdoherty · 2024-08-01T20:51:10Z

Context

v5 component refactor: TextArea #136 describes updates needed for the TextArea component to align with the Design System;
The key breaking change is that the TextArea can no longer be manually resized; rather, it should grow/shrink based on the amount of content entered.
I am leveraging an experimental CSS property, field-sizing to achieve the resizing behaviour for CSS-only and React consumers on Chrome and Edge browsers, with a fallback to a JS-based resizing solution for React-only consumers on other browsers.
This work is split across two PRs:
- 👉 Part 1: Add CSS resizing
- refactor: #136 TextArea - Add JS fallback for resizing #151

This PR (Part 1)

Moves __styles__/index.ts to styles.ts per the project structure conventions;
Updates CSS for ElTextArea styled component to:
- Use the newly available design tokens
- Expand the invalid state styling too be used either when either the elTextAreaHasError class name is applied, or the :invalid or :user-invalid pseudo-classes are active;
- Provide styling for :disabled and :read-only states;
- Provide max and min height constraints based on CSS variables that CSS-only consumers can easily override;
Updates ElTextArea to leverage experimental field-sizing property if supported;
Updates Storybook docs to demonstrate basic usage, invalid state, and resizing behaviour;
Updates TextArea and its prop interface to include minRows, maxRows and rows.

note: The design guidelines have not be finalised for this component, so there may be some additional changes once they are, particularly the treatment of default min/max rows values or the TextArea being resizable by default as opposed to fixed size by default.

note: This PR is on the large size, so if you'd like me to split it up further I can. In hindsight, the Storybook updates could have been left until later 😅

kurtdoherty · 2024-08-01T22:37:14Z

src/components/form-layout/form-layout.stories.tsx

@@ -77,7 +77,7 @@ export const BasicForm = {
        </InputWrap>
        <InputWrapFull>
          <InputGroup>


note: type is no longer an accepted prop for TextArea, so fixing up these stories.

Yep, agreed, there's no need

kurtdoherty · 2024-08-01T22:40:18Z

src/components/input-group/__styles__/index.ts

@@ -2,7 +2,7 @@ import { styled } from '@linaria/react'
 import { ElIcon } from '../../icon/__styles__'
 import { ElLabel } from '../../label/__styles__'
 import { ElInput, elHasInputError } from '../../input/__styles__'


note: Fixing up to import from the root textarea folder instead of relying on internal file structures.

kurtdoherty · 2024-08-01T22:40:38Z

src/components/textarea/__styles__/index.ts

note: Moved to textarea/styles.ts

kurtdoherty · 2024-08-01T22:43:36Z

src/components/textarea/__tests__/__snapshots__/textarea.test.tsx.snap

note: The current approach of snapshotting the render result is not ideal. It means we are capturing a host of React Testing Library's implementation details which are irrelevant to our component unit tests. As you'll see in the unit test file, I've updated the unit tests to snapshot the document fragment that contains our rendered component.

aside: It may be helpful to add this as a task to all the v5 refactor issues so that devs can remember to update the tests.

kurtdoherty · 2024-08-01T22:52:04Z

src/components/textarea/__tests__/textarea.test.tsx

@@ -1,9 +1,19 @@
-import { render } from '@testing-library/react'
+import { render, screen } from '@testing-library/react'
 import { TextArea } from '..'



note: I've updated the unit tests to avoid the generic "should match a snapshot". While this kind of unit test provides some value, it's missing out (IMO) on one of the most important aspects of unit tests: documentation. As such, I've replaced it with three tests that cover some of the important aspects we would want to document about our TextArea, though one is currently skipped (see code comment).

aside: I would not say that these unit tests are hitting the bar of "necessary and sufficient" by themselves, but the addition of the visual snapshots helps get us pretty close as they provide coverage of the invalid, disabled and read-only states, plus the initial sizing of text areas based on the minRows, maxRows and rows props.

I can provide more info on what the terms "necessary and sufficient" mean separately (they are terms we recently introduced to the Console FE team).

If you have some docs on this, would be more than happy to add to the wiki

kurtdoherty · 2024-08-01T22:55:41Z

src/components/textarea/styles.ts

+  '--textarea-min-rows': number
+}
+
+export const ElTextArea = styled.textarea`


note: The code comment below highlights an important decision I made in the implementation of this component. Instead of relying on Linaria's ability to convert ${({ maxRows }) => ...} into a randomly named CSS variable for us, I've opted to handle it manually in order to provide well-named CSS variables that CSS-only consumers can leverage.

I would have preferred to rely on data attributes on the textarea, but there's currently no way for CSS to use the value of a data attribute. See attr() for more information.

Yep, makes sense - appreciate the comments in here too

kurtdoherty · 2024-08-01T22:58:51Z

src/components/textarea/styles.ts

+  // a friendly interface for CSS-only consumers.
+  --textarea-max-rows: infinity;
+  --textarea-min-rows: 2;
+


note: The code comment below is another important decision. While I strongly believe there should be zero developer-defined, component-specific design tokens in Elements, I don't think that should prevent us from leveraging CSS variables in a "private" manner to aide the definition of our styles.

In order to try and communicate they are "private"/"implementation detail" CSS variables, I've prefixed them with the standard double underscore, __. Not sure if that's something we want to do or not.

If we'd prefer not to do this at all, I can remove these and just use the real design tokens directly in all the places they're referenced.

It does look a bit odd syntactically but the comment explains fine. I would suggest as long as they are outside the root scope it's probably fine.

@willmcvay I agree the syntax looks weird. We could choose to use private as a prefix? i.e. --private-textarea-border-width 🤔

kurtdoherty · 2024-08-02T00:08:31Z

src/components/textarea/styles.ts

+  @supports (field-sizing: content) {
+    field-sizing: content;
+  }
+


note: The code comment below highlights an important design decision I've made with the max and min height constraints for the text area. Since we want CSS-only consumers to get the correct resizing behaviour, we need to facilitate the max/min constraints in CSS, not JS.

I've used the min and max block-size CSS properties as they account for the consumer's writing-mode.

kurtdoherty · 2024-08-02T00:22:28Z

src/components/textarea/styles.ts

+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/writing-mode
+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/max-block-size
+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/min-block-size
+  max-block-size: calc(


note: We use the new lh unit to calculate the length of the rows, then add on the padding and border. See MDN's docs on CSS values and units for more information on lh, but the TL;DR is:

lh and rlh are relative lengths units similar to em and rem. The difference between lh and rlh is that the first one is relative to the line height of the element itself, while the second one is relative to the line height of the root element, usually <html>.

I did just check this out in Safari just because Safari and couldn't see any issues FYI

kurtdoherty · 2024-08-02T00:23:19Z

src/components/textarea/styles.ts

+    outline: none;
+    border-color: var(--outline-text_input-focus);
+  }
+


note: the original Elements text area did not have styles for disabled or read-only states. These have been added here. The Design System does not currently differentiate visually between disable and read-only; this may change in future.

kurtdoherty · 2024-08-02T00:25:00Z

src/components/textarea/styles.ts

+  &:disabled {
+    background-color: var(--fill-default-lightest);
+  }
+


note: The original text area only applied the invalid state styles when the elTextAreaHasError class was present. This has been expanded to include the :invalid and :user-invalid pseudo-classes.

kurtdoherty · 2024-08-02T00:27:12Z

src/components/textarea/textarea.mdx

note: Some minor updates here to include the extra stories that have been added.

aside: I'm keen to understand why Elements prefers MDX files over Storybook's autogenerated DocsPage functionality. What I'm doing here (e.g. the use of the Description component to render the JS Doc comments from the component or a specific story) is all handled by Storybook's DocsPage feature, so I'm not sure what value we are getting from doing this ourselves 🤔 Is it primarily to facilitate the RenderHtmlMarkup component?

kurtdoherty · 2024-08-02T00:28:35Z

src/components/textarea/textarea.stories.tsx

note: I've expanded the number of stories being demonstrated here, as well as applied Storybook's types to the exports. The JS Doc for each story is picked up by Storybook's Description component, which is now in use in textarea.mdx.

kurtdoherty · 2024-08-02T00:33:09Z

src/components/textarea/textarea.tsx

+
+// NOTE: We omit the `cols` prop because our text area should always grow to the width of its container.
+interface RestrictedTextareaHTMLAttributes extends Omit<TextareaHTMLAttributes<HTMLTextAreaElement>, 'cols'> {}
+


note: I've updated how the component's prop interface is defined for two reasons:

To add the new minRows and maxRows props; and,

To use the TextareaHTMLAttributes instead of InputHTMLAttributes (the latter one is for <input> elements).

kurtdoherty · 2024-08-02T00:34:32Z

src/components/textarea/textarea.tsx

+ * resizing behaviour is available for CSS-only consumers on Chrome and Edge. For browsers that do not yet
+ * support the [field-sizing](https://developer.mozilla.org/en-US/docs/Web/CSS/field-sizing) property, we
+ * fallback to a JS-based resizing solution that is only available to React-based consumers.
+ */


note: I've opted to use forwardRef's generic type slots instead of the original TextAreaWrapped type. This makes for a simpler approach and ensures our forwarded ref is correctly typed.

kurtdoherty · 2024-08-02T00:36:33Z

src/components/textarea/textarea.tsx

+            // NOTE: `rows` takes precedence. If it's defined, the text area is effectively fixed-height.
+            // If it's not defined, we allow resizing to occur between the min and max rows.
+            '--textarea-max-rows': rows ?? maxRows,
+            '--textarea-min-rows': rows ?? minRows,


note: I'm using satisfies TextAreaCSSProperties (a type which comes from our styles.ts module) in order to ensure we don't provide more styles here than what we explicitly permit through the TextAreaCSSProperties.

I then assert this to be the CSSProperties type in order to satisfy React/Linaria's expectations (they don't consider the CSS variables to be valid property names).

willmcvay

Hi @kurtdoherty so looks good to me - have tested locally in Safari, FF & Chromium because of some of the edgier properties and works as expected which is great - will check out the JS fallback next.

Comments are appreciated for the more non-vanilla implementations - I appreciate, it's a tricky bit of code.

Just that small issue with the stories file to move to the new structure in SB but happy if you want to move that to the second PR.

willmcvay · 2024-08-02T13:41:21Z

src/components/form-layout/form-layout.stories.tsx

@@ -77,7 +77,7 @@ export const BasicForm = {
        </InputWrap>
        <InputWrapFull>
          <InputGroup>


Yep, agreed, there's no need

willmcvay · 2024-08-02T13:41:56Z

src/components/textarea/__tests__/__snapshots__/textarea.test.tsx.snap

-  "rerender": [Function],
-  "unmount": [Function],
-}
+exports[`can set custom min/max rows 1`] = `


Much cleaner without the React internals :-)

willmcvay · 2024-08-02T13:42:55Z

src/components/textarea/__tests__/textarea.test.tsx

@@ -1,9 +1,19 @@
-import { render } from '@testing-library/react'
+import { render, screen } from '@testing-library/react'
 import { TextArea } from '..'



If you have some docs on this, would be more than happy to add to the wiki

willmcvay · 2024-08-02T13:46:17Z

src/components/textarea/styles.ts

+  '--textarea-min-rows': number
+}
+
+export const ElTextArea = styled.textarea`


Yep, makes sense - appreciate the comments in here too

willmcvay · 2024-08-02T13:48:15Z

src/components/textarea/styles.ts

+  // a friendly interface for CSS-only consumers.
+  --textarea-max-rows: infinity;
+  --textarea-min-rows: 2;
+


It does look a bit odd syntactically but the comment explains fine. I would suggest as long as they are outside the root scope it's probably fine.

willmcvay · 2024-08-02T13:51:08Z

src/components/textarea/styles.ts

+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/writing-mode
+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/max-block-size
+  // @see https://developer.mozilla.org/en-US/docs/Web/CSS/min-block-size
+  max-block-size: calc(


I did just check this out in Safari just because Safari and couldn't see any issues FYI

src/components/textarea/textarea.stories.tsx

adrian-reapit · 2024-08-05T17:02:50Z

With the other PR this is quite an update on a normal TextArea and if its got bespoke 'tricky' code it might better to make a new component. MaterialUI has a TextareaAutosize component, name makes clear what you are getting https://mui.com/base-ui/react-textarea-autosize/

kurtdoherty · 2024-08-05T22:30:30Z

With the other PR this is quite an update on a normal TextArea and if its got bespoke 'tricky' code it might better to make a new component. MaterialUI has a TextareaAutosize component, name makes clear what you are getting https://mui.com/base-ui/react-textarea-autosize/

@adrian-reapit I did consider that option originally, but decided to take the approach I've presented in these PRs because:

We've previously discussed as a DS working group that we want to aim for a 1-1 equivalency between Elements and the Design System. While we shouldn't blindly adhere to that in every case, I do think it makes sense for TextArea;
The Design System has no desire for a text area that can be manually resized. They will either be fixed height or will automatically resize between a min and max number of lines;
I view the JS-based fallback for resizing to be (relatively) short-lived. Once field-sizing support is rolled out for Safari (it's already in progress) and Firefox (its on their radar), all the "tricky" code will disappear;
Once the JS-based resizing logic is removed, the idea of having two text area components whose only difference is the field-sizing CSS property doesn't seem ideal; and,
I really value the DX of seeing a design use a TextArea component and being able to reach for a component with the same name and the same props.

willmcvay · 2024-08-06T14:35:03Z

@kurtdoherty @adrian-reapit Just to resolve the slight log-jam, why not take the "Styles Only" usage approach and export a wrapped version of ElTextArea like we do with the Accordion atoms. Then people can choose between batteries included or simple HTML eg https://github.com/reapit/elements/blob/main/src/components/accordion/accordion.atoms.tsx#L12

kurtdoherty · 2024-08-06T22:12:55Z

@willmcvay we already are exporting the basic styled component, ElTextArea. You can see this happening in TextArea/styles.ts and TextArea/index.ts. 👍

I'm not sure there's much value in exporting another component that simply wraps it? The atom would want to be called TextArea, but that's the name of the "main" component too. This isn't a problem with the Accordion atoms because they're all uniquely named 🤔

kurtdoherty · 2024-08-06T22:36:02Z

note: I have a meeting with Andrei today to talk through the component interface, particularly around what design would like to see as the properties for their TextArea in Figma and how that works for us, so there may be some minor changes land after that conversation.

willmcvay · 2024-08-07T09:30:09Z

@willmcvay we already are exporting the basic styled component, ElTextArea. You can see this happening in TextArea/styles.ts and TextArea/index.ts. 👍

I'm not sure there's much value in exporting another component that simply wraps it? The atom would want to be called TextArea, but that's the name of the "main" component too. This isn't a problem with the Accordion atoms because they're all uniquely named 🤔

Will leave up to you. IMO, I am in favour of the implementation you presented - was just a suggestion to alleviate any concerns Adrian had. Will leave you to guys to agree a preference. If the auto-resizing behaviour is what the design team want, I am very much on board with delivering this.

kurtdoherty · 2024-08-07T10:47:54Z

question: @adrian-reapit just checking in to see if your concerns about the approach taken in this PR and #151 are "blocking", or if you're okay for me to proceed? If they're blocking, I'd like to line up a video call for us to chat through our options as soon as possible so we can get this work finished off and merged in 🙏

kurtdoherty · 2024-08-08T22:59:22Z

From @adrian-reapit via a Teams chat:

Its not blocking but iIs a noticeable change.. I would just give the choice until it had full browser support and didn't need a fallback. We have 12 uses of textarea in our application which will need to be checked so that min/max are set correctly and our site is all mobile usage so the 'fallback' is the component. Everyone updating to v5 will need to do the same on top of all other changes. testers would need to know that the desktop version is not the same as mobile. Happy to discuss if you want

I appreciate the concern around the adoption/migration cost for v4 consumers. Console Cloud has a significant migration ahead of it too. A few thoughts though:

The Design System wants text area's to have a minimum of 3 rows but no maximum number of rows, by default. This means those 12 text areas would largely behave the same as they do now, only they'll resize themselves, not allow the user to do that themselves. Conceptually, this seems like a pretty safe change for those 12 text areas, but I acknowledge there may be nuances to those 12 uses that I'm not aware of that make this change more concerning.
Off the back of a conversation with Andrei, I need to tweak the component interface by adding a new prop, fieldSizing with two values fixed and content. While content would be the default value for this prop, we could add a third, deprecated option, manual, that would restore the manual resizing behaviour the v4 text area currently has. This would mean v4 consumers that want to maintain this existing behaviour would just need to add fieldSizing="manual". Would this be reasonable?

kurtdoherty · 2024-08-12T02:12:59Z

From @adrian-reapit via Teams chat:

That suggestion would be ok. Thanks

Done 👍

I've introduced the new fieldSizing: content | fixed | manual prop as per designs feedback (and yours re manual sizing). Merging this PR in now, so that we can get this work into final review with design.

kurtdoherty added the v5 Issues for v5 release label Aug 1, 2024

kurtdoherty force-pushed the 136-v5-component-refactor-textarea branch from a81340c to a1ece07 Compare August 1, 2024 22:09

kurtdoherty commented Aug 1, 2024

View reviewed changes

src/components/textarea/__styles__/index.ts Outdated

Copy link

Contributor Author

kurtdoherty Aug 1, 2024

Choose a reason for hiding this comment

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

note: Moved to textarea/styles.ts

kurtdoherty commented Aug 1, 2024

View reviewed changes

kurtdoherty force-pushed the 136-v5-component-refactor-textarea branch from a1ece07 to c7fdf7f Compare August 2, 2024 00:03

kurtdoherty commented Aug 2, 2024

View reviewed changes

kurtdoherty force-pushed the 136-v5-component-refactor-textarea branch from c7fdf7f to 8db308a Compare August 2, 2024 00:18

kurtdoherty commented Aug 2, 2024

View reviewed changes

kurtdoherty requested review from willmcvay and bashleigh August 2, 2024 00:37

kurtdoherty marked this pull request as ready for review August 2, 2024 00:44

kurtdoherty mentioned this pull request Aug 2, 2024

refactor: #136 TextArea - Add JS fallback for resizing #151

Merged

kurtdoherty changed the title ~~refactor: #136 TextArea (part 1)~~ refactor: #136 TextArea - Add CSS resizing Aug 2, 2024

willmcvay approved these changes Aug 2, 2024

View reviewed changes

kurtdoherty added 2 commits August 12, 2024 08:23

v5 component refactor: #136 TextArea (part 1)

40d481a

PR feedback rnd. 1

fe4a312

kurtdoherty force-pushed the 136-v5-component-refactor-textarea branch from 844f2d4 to 509ae02 Compare August 11, 2024 23:27

PR feedback rnd. 2

2f8e95e

kurtdoherty force-pushed the 136-v5-component-refactor-textarea branch from 509ae02 to 2f8e95e Compare August 12, 2024 02:09

kurtdoherty merged commit a24905a into main Aug 12, 2024
4 checks passed

kurtdoherty deleted the 136-v5-component-refactor-textarea branch August 12, 2024 02:15

kurtdoherty mentioned this pull request Aug 21, 2024

Avoid capturing RTL internals in DOM snapshots #157

Open


		// NOTE: We omit the `cols` prop because our text area should always grow to the width of its container.
		interface RestrictedTextareaHTMLAttributes extends Omit<TextareaHTMLAttributes<HTMLTextAreaElement>, 'cols'> {}

refactor: #136 TextArea - Add CSS resizing #150

refactor: #136 TextArea - Add CSS resizing #150

Conversation

kurtdoherty commented Aug 1, 2024 • edited Loading

Context

This PR (Part 1)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtdoherty Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtdoherty Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

willmcvay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-reapit commented Aug 5, 2024

kurtdoherty commented Aug 5, 2024 • edited Loading

willmcvay commented Aug 6, 2024

kurtdoherty commented Aug 6, 2024 • edited Loading

kurtdoherty commented Aug 6, 2024

willmcvay commented Aug 7, 2024

kurtdoherty commented Aug 7, 2024

kurtdoherty commented Aug 8, 2024 • edited Loading

kurtdoherty commented Aug 12, 2024 • edited Loading

kurtdoherty commented Aug 1, 2024 •

edited

Loading

kurtdoherty Aug 2, 2024 •

edited

Loading

kurtdoherty Aug 2, 2024 •

edited

Loading

kurtdoherty commented Aug 5, 2024 •

edited

Loading

kurtdoherty commented Aug 6, 2024 •

edited

Loading

kurtdoherty commented Aug 8, 2024 •

edited

Loading

kurtdoherty commented Aug 12, 2024 •

edited

Loading