feat(TextArea): enable errorplacement prop ISSUE-2270#2689
Open
KrotovPetr wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideImplements an File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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', |
Contributor
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);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#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: