feat(TextArea): enable errorplacement prop ISSUE-2270#2689
Conversation
Reviewer's GuideImplements an File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Preview is ready. |
|
🎭 Component Tests Report is ready. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new inside error icon/tooltip flow doesn’t appear to expose the error text to assistive technologies (no
aria-describedbyfrom the textarea and no ARIA attributes on the icon/tooltip), so consider wiring the tooltip content into the existing accessibility path used for outside errors to keep the control screen‑reader friendly. - The error icon trigger is currently a plain
<span>inside aPopover; consider giving it a semantic role (e.g.,role="button"andtabIndex=0, or making it purely decorative and reflecting the error on the textarea itself) so keyboard users can reliably access the tooltip or aren’t required to interact with it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new inside error icon/tooltip flow doesn’t appear to expose the error text to assistive technologies (no `aria-describedby` from the textarea and no ARIA attributes on the icon/tooltip), so consider wiring the tooltip content into the existing accessibility path used for outside errors to keep the control screen‑reader friendly.
- The error icon trigger is currently a plain `<span>` inside a `Popover`; consider giving it a semantic role (e.g., `role="button"` and `tabIndex=0`, or making it purely decorative and reflecting the error on the textarea itself) so keyboard users can reliably access the tooltip or aren’t required to interact with it.
## Individual Comments
### Comment 1
<location path="src/components/controls/TextArea/__tests__/TextArea.visual.test.tsx" line_range="124" />
<code_context>
});
+
+ test(
+ 'smoke inside error placement tooltip',
+ {tag: ['@smoke']},
+ async ({mount, page, expectScreenshot}) => {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the smoke tooltip test by asserting the tooltip content text, not just visibility
Right now the test only checks that `.g-popup` becomes visible after hovering the error icon, so it won’t catch cases where the tooltip shows the wrong text. Please also assert that the popup contains `Test error message` (or at least the `errorMessage` prop value) to better guard against regressions in tooltip content.
Suggested implementation:
```typescript
'smoke inside error placement tooltip',
{tag: ['@smoke']},
async ({mount, page, expectScreenshot}) => {
const props: TextAreaProps = {
...defaultProps,
value: 'Text',
validationState: 'invalid',
errorMessage: 'Test error message',
errorPlacement: 'inside',
};
const root = await mount(
<div style={{width: 250}}>
```
```typescript
const popup = page.locator('.g-popup');
await expect(popup).toBeVisible();
await expect(popup).toContainText('Test error message');
```
If the test currently uses a different selector or assertion for the tooltip (for example, a more specific locator than `.g-popup`, or `toHaveText` instead of `toBeVisible`), adapt the `popup` locator and the added `toContainText('Test error message')` call accordingly.
If `errorMessage` is changed in this test in the future, consider asserting `props.errorMessage` instead of the literal string to keep the test in sync:
```ts
await expect(popup).toContainText(props.errorMessage);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
|
|
||
| test( | ||
| 'smoke inside error placement tooltip', |
There was a problem hiding this comment.
suggestion (testing): Strengthen the smoke tooltip test by asserting the tooltip content text, not just visibility
Right now the test only checks that .g-popup becomes visible after hovering the error icon, so it won’t catch cases where the tooltip shows the wrong text. Please also assert that the popup contains Test error message (or at least the errorMessage prop value) to better guard against regressions in tooltip content.
Suggested implementation:
'smoke inside error placement tooltip',
{tag: ['@smoke']},
async ({mount, page, expectScreenshot}) => {
const props: TextAreaProps = {
...defaultProps,
value: 'Text',
validationState: 'invalid',
errorMessage: 'Test error message',
errorPlacement: 'inside',
};
const root = await mount(
<div style={{width: 250}}> const popup = page.locator('.g-popup');
await expect(popup).toBeVisible();
await expect(popup).toContainText('Test error message');If the test currently uses a different selector or assertion for the tooltip (for example, a more specific locator than .g-popup, or toHaveText instead of toBeVisible), adapt the popup locator and the added toContainText('Test error message') call accordingly.
If errorMessage is changed in this test in the future, consider asserting props.errorMessage instead of the literal string to keep the test in sync:
await expect(popup).toContainText(props.errorMessage);
| &_has-clear#{$block}_has-error-icon { | ||
| #{$block}__clear { | ||
| inset-inline-end: calc(var(--_--clear-offset) + 20px); | ||
| } | ||
|
|
||
| &#{$block}_has-scrollbar #{$block}__clear { | ||
| inset-inline-end: calc(var(--g-scrollbar-width) + 20px); | ||
| } | ||
| } |
There was a problem hiding this comment.
[minor] On l/xl the error icon overlaps the clear button (~4px hit-box overlap).
Here the clear button is pushed to a hardcoded clear-offset + 20px (≈22px). But the icon's far edge is error-icon-offset-inline + 16px: for s/m that's 6+16=22px (flush, ok), while for l/xl it's 10+16=26px, i.e. it slides ~4px under the clear button. The +20px only matches the s/m offset. This is invisible in review because the showcase and both visual tests use size m.
I'd tie the offset to error-icon-offset-inline so the clear button lands exactly at the icon's far edge for every size (s/m unchanged: 6+16=22; l/xl: 10+16=26):
| &_has-clear#{$block}_has-error-icon { | |
| #{$block}__clear { | |
| inset-inline-end: calc(var(--_--clear-offset) + 20px); | |
| } | |
| &#{$block}_has-scrollbar #{$block}__clear { | |
| inset-inline-end: calc(var(--g-scrollbar-width) + 20px); | |
| } | |
| } | |
| &_has-clear#{$block}_has-error-icon { | |
| #{$block}__clear { | |
| inset-inline-end: calc(var(--_--clear-offset) + var(--_--error-icon-offset-inline) + 14px); | |
| } | |
| &#{$block}_has-scrollbar #{$block}__clear { | |
| inset-inline-end: calc(var(--g-scrollbar-width) + var(--_--error-icon-offset-inline) + 14px); | |
| } | |
| } |
Would also be good to add a visual scenario for l/xl + hasClear + errorPlacement="inside" so this case is screenshot-covered.
| &__error-icon-wrap { | ||
| position: absolute; | ||
| inset-inline-end: var(--_--error-icon-offset-inline); | ||
| inset-block-start: var(--_--error-icon-offset-block); | ||
| } |
There was a problem hiding this comment.
[nit / robustness] Equal-specificity position conflict relies on stylesheet source order.
className={b('error-icon-wrap')} (TextArea.tsx:188) lands on the legacy Popover's root <div>, so the same element carries both .g-popover-legacy { position: relative } and .g-text-area__error-icon-wrap { position: absolute } — both (0,1,0) selectors. absolute only wins because TextArea's styles are emitted after the Popover's; a change in import/bundler order could silently break the positioning. TextInput avoids this by not passing className to its Popover.
Simplest fix — raise the rule's specificity so it wins deterministically:
| &__error-icon-wrap { | |
| position: absolute; | |
| inset-inline-end: var(--_--error-icon-offset-inline); | |
| inset-block-start: var(--_--error-icon-offset-block); | |
| } | |
| &__error-icon-wrap#{$block}__error-icon-wrap { | |
| position: absolute; | |
| inset-inline-end: var(--_--error-icon-offset-inline); | |
| inset-block-start: var(--_--error-icon-offset-block); | |
| } |
A cleaner alternative — don't put the class on the Popover at all; wrap it in a dedicated <span className={b('error-icon-wrap')}> and leave the Popover with its position: relative.
| &#{$block}_has-clear #{$block}__control, | ||
| &#{$block}_has-error-icon #{$block}__control { | ||
| padding-inline-end: 26px; |
There was a problem hiding this comment.
[minor] view="clear" + errorPlacement="inside" keeps the control's 26/36px end padding.
These _has-error-icon __control { padding-inline-end } rules have specificity (0,3,0), while the clear-view reset .g-text-area_view_clear .g-text-area__control { padding-inline: 0 } is (0,2,0), so it loses: in clear view the text doesn't sit flush as expected.
I'd scope the per-size padding under view_normal so it doesn't fight the clear-view reset, e.g.:
&#{$block}_view_normal#{$block}_has-clear #{$block}__control,
&#{$block}_view_normal#{$block}_has-error-icon #{$block}__control {
padding-inline-end: 26px;
}(same for 46px, and for l/xl). Alternatively, zero out --_--error-icon-offset-inline and the padding inside the &_view_clear block. And add a clear+inside screenshot. Affects all sizes: s (≈113), m (132), l (152), xl (172).
|
|
||
| &_has-clear#{$block}_has-error-icon { | ||
| #{$block}__clear { | ||
| inset-inline-end: calc(var(--_--clear-offset) + 20px); |
There was a problem hiding this comment.
[nit] On l/xl the clear button loses its +1px horizontal nudge when the icon appears.
The base clear button on l/xl sits at calc(var(--_--clear-offset) + 1px), but this override (and its scrollbar variant on line 242) uses + 20px without the +1, causing a 1px horizontal jitter when the icon shows up. Vertical is fine (inset-block-start is untouched).
If the offset-inline-based formula from the overlap comment is adopted, positioning becomes consistent across sizes and this point essentially goes away. Otherwise, carry the per-size +1px via a dedicated variable. Low priority.
| &__error-icon-wrap { | ||
| position: absolute; | ||
| inset-inline-end: var(--_--error-icon-offset-inline); | ||
| inset-block-start: var(--_--error-icon-offset-block); |
There was a problem hiding this comment.
[nit] The icon's vertical center is ~1px off from the clear button's center.
The clear button is centered exactly on the control's vertical center at every size, but because of the hardcoded --_--error-icon-offset-block (5/7/11/15px) the icon's center is 1px lower on m/l/xl and 1px higher on s. Only noticeable when both the clear button and the inside icon are shown.
Cleaner to center generically instead of the per-size hardcode (then the --_--error-icon-offset-block variables can be dropped):
&__error-icon-wrap {
/* ... */
inset-block-start: 50%;
transform: translateY(-50%);
}This mirrors the auto-centering TextInput already uses (flex). NB: a blanket -1px on the offsets won't work — s would get worse.
|
5fc89b1 to
d66a7c8
Compare
korvin89
left a comment
korvin89
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — align the inside error icon (errorPlacement="inside") with TextInput.
After the fix the measurements match on both axes for every size (centerY 12/14/18/22, right-gap 5/5/9/13).
| &__error-icon-wrap { | ||
| position: absolute; | ||
| display: inline-flex; | ||
| inset-inline-end: 0; |
There was a problem hiding this comment.
🤖 AI-generated suggestion
The wrap is pinned to the outer edge (outside the 1px border), so the icon sits 1px closer to the right edge than in TextInput. Offsetting by the border width aligns them pixel-perfectly.
| inset-inline-end: 0; | |
| inset-inline-end: var(--g-text-area-border-width, var(--_--border-width)); |
| padding-inline-end: 42px; | ||
| } | ||
|
|
||
| --_--error-icon-padding-block: 5px; |
There was a problem hiding this comment.
🤖 AI-generated suggestion
padding-block is 1px short of true centering, so the glyph sits 1px above center (size s). +1px matches the glyph center to the field center, like TextInput.
| --_--error-icon-padding-block: 5px; | |
| --_--error-icon-padding-block: 6px; |
| padding-inline-end: 48px; | ||
| } | ||
|
|
||
| --_--error-icon-padding-block: 5px; |
There was a problem hiding this comment.
🤖 AI-generated suggestion
Same as size s: the error icon's vertical centering is 1px short (size m).
| --_--error-icon-padding-block: 5px; | |
| --_--error-icon-padding-block: 6px; |
| padding-inline-end: 62px; | ||
| } | ||
|
|
||
| --_--error-icon-padding-block: 9px; |
There was a problem hiding this comment.
🤖 AI-generated suggestion
The error icon's vertical centering is 1px short (size l).
| --_--error-icon-padding-block: 9px; | |
| --_--error-icon-padding-block: 10px; |
| padding-inline-end: 66px; | ||
| } | ||
|
|
||
| --_--error-icon-padding-block: 13px; |
There was a problem hiding this comment.
🤖 AI-generated suggestion
The error icon's vertical centering is 1px short (size xl).
| --_--error-icon-padding-block: 13px; | |
| --_--error-icon-padding-block: 14px; |
| } | ||
|
|
||
| #{$block}__error-icon-wrap { | ||
| inset-inline-end: var(--g-scrollbar-width); |
There was a problem hiding this comment.
🤖 AI-generated suggestion
With a scrollbar the icon must also be offset by the border width so the right-side gap matches TextInput.
| inset-inline-end: var(--g-scrollbar-width); | |
| inset-inline-end: calc( | |
| var(--g-scrollbar-width) + var(--g-text-area-border-width, var(--_--border-width)) | |
| ); |
2 participants
#2270
Summary by Sourcery
Add support for placing TextArea validation errors inside the control via an icon tooltip and update styles, tests, and docs accordingly.
New Features:
Documentation:
Tests: