-
Notifications
You must be signed in to change notification settings - Fork 0
Issue 44 add textarea component #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small changes then feel free to merge.
Good work as per usual!
| // placeholder: text color: --bloom-gray | ||
|
|
||
| "use client"; | ||
| import React, { useState } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused useState
| placeholder?: string; | ||
| rows?: number; | ||
| value: string; | ||
| setValue: (value: string) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better API consistency we can change setValue to onChange
| "use client"; | ||
| import React, { useState } from "react"; | ||
|
|
||
| interface TextareaProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency / extra styling add these extra fields:
required?:- The text area might be a required field in a formclassname?:- The text area might need extra classes in some use cases
…ssNames props to allow customization flexibility.
Fozzyack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Eden, sorry I should have been a bit clearer haha. The className and required field should be just for the textarea element - no need to worry about the label customisation. We can also simplify the component down by removing the div and moving some of those class names to the textarea.
Something like this: (one className prop, one required prop)
const Textarea: React.FC<TextareaProps> = ({
id,
name,
label,
placeholder,
rows,
value,
onChange,
required,
className,
}) => {
return (
<>
<label htmlFor={name} className="body-sm-bold mb-1 block">
{label ? label : capitalizeFirstLetter(name)}
{required && <span className="text-bloom-red"> *</span>}
</label>
<textarea
id={id ? id : name}
name={name}
placeholder={placeholder ? placeholder : capitalizeFirstLetter(name)}
rows={rows ? rows : 4}
className={cn(
"body w-full rounded-md border border-[hsl(var(--border))] bg-background shadow-[0_4px_0_0_#D1D5DB] px-3 py-2 outline-none placeholder:text-bloom-gray",
className,
)}
value={value}
onChange={(e) => onChange(e.target.value)}
/>
</>
);
};| <span | ||
| className={cn("text-[var(--bloom-red)]", requiredTextClassName)} | ||
| > | ||
| {requiredText ? requiredText : "*"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the required field we can just use something like {required && <span className="text-[var(--bloom-red)]"> *</span>}.
| placeholder={placeholder ? placeholder : capitalizeFirstLetter(name)} | ||
| rows={rows ? rows : 4} | ||
| className={cn( | ||
| "body w-full bg-transparent px-3 py-2 outline-none placeholder:text-[var(--bloom-gray)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[placerholder](placeholder:text-[var(--bloom-gray)]") can be replaced with text-bloom-gray
| </label> | ||
| <div | ||
| className={cn( | ||
| "w-full rounded-md border border-[hsl(var(--border))] bg-background shadow-[0_4px_0_0_#D1D5DB]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can actually remove this div and give some of these classnames to the textarea
…e test and documentation.
Fozzyack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent 👍
Change Summary
Add textarea component
Change Form
Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.
Other Information
Hi reviewer! I'm also not sure if it meets the requirement about shadcn ui, but since I dont see any package named shadcn in the package.json so I just observe how the current files work. Please let me know if its not an appropriate implementation
textarea_demo.mp4
Related issue