feat: input and label components #70
Conversation
){kind=icon}
){kind=icon}
){kind=icon}
){kind=icon}
src/components/input.tsx
Outdated
| loading: { | ||
| true: 'text-transparent', | ||
| }, |
There was a problem hiding this comment.
Why the text needs to be transparent in the loading state?
There was a problem hiding this comment.
leftover - in this case we will handle the loading with disabled state which behaves the same
| type?: 'text' | 'password' | 'email' | 'number' | ||
| } | ||
|
|
||
| export const Input = forwardRef<HTMLInputElement, InputProps>(function Input( |
There was a problem hiding this comment.
If we pass an error message we need to:
- render the error message below the input
- add the
aria-invalidandaria-describedbyattributes to the input
Additionally we can use the label component directly, by adding a label prop:
<Input label="Amount" {...restOfProps} />There was a problem hiding this comment.
we must use it separately because there is a situation when we have also a link besides the actualy label (as per graphic)
e.g. Label link
| ) { | ||
| return ( | ||
| <div className="relative"> | ||
| {icon && <div className="absolute left-4 top-4">{icon}</div>} |
There was a problem hiding this comment.
Are we satisfied with the absolute positioning for the icon or should we inline it with the input?
There was a problem hiding this comment.
I think it's fine for now, I don't see yet an advantage of using it inline 🤔
| it('should render an input with the `aria-label` attribute', () => { | ||
| const { queryByLabelText } = render(<Input aria-label="test input" />) | ||
|
|
||
| expect(queryByLabelText('test input')).toBeInTheDocument() | ||
| expect(queryByLabelText('test input')).toHaveAttribute('aria-label', 'test input') | ||
| }) |
There was a problem hiding this comment.
I don't think we need to the aria-label attribute for the input, since the label will be available in the DOM.
| it('should have the `border-error` class when the `error` variant is passed', () => { | ||
| const { queryByLabelText } = render(<Input aria-label="test input" error />) | ||
|
|
||
| expect(queryByLabelText('test input')).toBeInTheDocument() | ||
| expect(queryByLabelText('test input')).toHaveClass('border-error') | ||
| }) |
There was a problem hiding this comment.
Let's also check if the error message is present in the DOM and if the input has the aria-invalid and aria-labeledby attributes.
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
github-actions
bot
added
the
area: ci
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
Co-authored-by: Radu-Cristian Popa <[email protected]>
…ion into rp--input-component
…tization-extension into rp--input-component
github-actions
bot
removed
the
area: ci
src/components/input.tsx
Outdated
| className={cn(inputVariants({ disabled }), icon && 'pl-12', className)} | ||
| disabled={disabled ?? false} | ||
| aria-disabled={disabled ?? false} | ||
| aria-invalid={!!errorMessage || undefined} |
There was a problem hiding this comment.
I think that !!errorMessage should be sufficient.
src/components/input.tsx
Outdated
| <div className="absolute top-full left-0 right-0 text-error text-sm px-2"> | ||
| {errorMessage} | ||
| </div> |
There was a problem hiding this comment.
Let's not set absolute position to the error message. A simple p tag should work.
src/components/input.tsx
Outdated
| aria-describedby={errorMessage} | ||
| {...props} | ||
| /> | ||
| {errorMessage && <p>{errorMessage}</p>} |
There was a problem hiding this comment.
Only the absolute position styles needed to be removed. Let's keep the other ones.
Closes #50
Changes proposed in this pull request