feat: add text input component with tests and stories#25
feat: add text input component with tests and stories#25
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Why did you leave the template of PR almost blank? No changes, no testing description... Actually, you didn't write anything. We defined such a format, not github. We expect you to fill it. |
whit33y
left a comment
There was a problem hiding this comment.
Few bugs that I found right now. I will do further more accurate review later.
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
whit33y
left a comment
There was a problem hiding this comment.
You resolved bugs, but there is still place for improvement.
Just take a look at figma and styles.
https://www.figma.com/design/yZHdiIjMNUNJGyj9OLUpsz/athena?node-id=145-259&t=En2zw1gUl3BH3Uwj-4
src/components/ui/input.tsx
Outdated
|
|
||
| function Input({ className, type, ...props }: React.ComponentProps<"input">) { | ||
| return ( | ||
| <input |
There was a problem hiding this comment.
suggestion: I don't think that the input.tsx is needed, the idea was to improve it. So I think it should be deleted becuase we have TextInput component right now
| <span className="text-sm text-muted-foreground">{prefix}</span> | ||
| )} | ||
|
|
||
| <Input |
There was a problem hiding this comment.
suggestion: I think this should look like this:
<input
id={inputId}
type={type}
value={value}
onChange={handleChange}
placeholder={placeholder}
required={required}
disabled={disabled}
data-test-id={dataTestId}
aria-invalid={isError || undefined}
className={cn("flex-1 border-0 bg-transparent px-0 py-0", className)}
{...inputProps}
/>
Instead of importing the basic version of shadcn input this component supposed to it's impoved version
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
| const generatedId = React.useId(); | ||
| const inputId = id ?? generatedId; | ||
|
|
||
| const showError = Boolean(isError && errorText); |
There was a problem hiding this comment.
suggestion:
showError uses Boolean(isError && errorText) but aria-invalid ( line ) uses isError directly.
You can end up with aria-invalid="true" but displaying helper text instead of an error, which is confusing for assistive tech. It would be better to stick with one solution
| const required = isRequired ?? inputProps.required; | ||
|
|
||
| const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (disabled) return; |
There was a problem hiding this comment.
suggestion:
handleChange early-returns when disabled is true. This is correct behavior, but because the input itself is already disabled, the handler won’t fire anyway; the guard is redundant.
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
| const showError = isError; | ||
| const disabled = isDisabled ?? inputProps.disabled; | ||
| const required = isRequired ?? inputProps.required; | ||
| const disabled = isDisabled |
There was a problem hiding this comment.
suggestion:
I don't think it's necessary to assign value of isDisabled and isRequired props to consts they can be passed straight were they should be used
There was a problem hiding this comment.
suggestion:
try to provide more information in your commits names for example what has been changed, was it a feature, was it a fix.
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Signed-off-by: kuba kesler <keslerkuba12@gmail.com>
Coverage ReportCoverage after merging
|
Changes
What has been done in this PR?
How to test
Describe how to test the changes manually
Checklist
Check if the code follows the standards
console.log/ leftover comments