-
Notifications
You must be signed in to change notification settings - Fork 8
Feature: Add form components #106
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
base: develop
Are you sure you want to change the base?
Conversation
|
Apologies! I realized the package-lock.json was needed. I've restored it now. |
| label: { control: "text" }, | ||
| placeholder: { control: "text" }, | ||
| disabled: { control: "boolean" }, | ||
| variant: { control: "select", options: ["outline", "subtle"] }, | ||
| size: { control: "select", options: ["sm", "md", "lg", "xl"] }, | ||
| rows: { control: "number" }, |
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.
Please include a descriptions as well for the argTypes properties.
| value: string; | ||
| label: string; | ||
| disabled?: boolean; | ||
| [key: string]: any; // Allow extra props |
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.
Do we need this type? We should avoid using overly general types as much as possible.
| variant?: SelectVariant; | ||
| disabled?: boolean; | ||
| value?: string; | ||
| value?: SelectOption; // Changed from string to object for Headless UI |
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.
General feedback for all the code: remove any unnecessary or overly specific comments. Comments should only be added when the code is not self-explanatory.
| // 1. Define valid keys for casting | ||
| type SizeKey = "sm" | "md" | "lg" | "xl"; | ||
| type VariantKey = "subtle" | "outline" | "ghost"; |
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.
Types should be located in the types.ts file.
| // --- Styles --- | ||
| // Fix 1: Cast 'size' to SizeKey | ||
| const sizeClasses = { |
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.
| style?: Record<string, string | number | boolean>; | ||
| } | ||
| style?: CSSProperties; // Improved type | ||
| [key: string]: any; // Allow other standard input props (onBlur, onFocus, etc.) |
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.
Instead of allowing any types, please extend prop type with a proper type such as React.InputHTMLAttributes<HTMLInputElement>.
| error?: string; // Added | ||
| onChange?: (event: ChangeEvent<HTMLInputElement>) => void; | ||
| prefix?: ReactNode; // Simplified type | ||
| suffix?: ReactNode; // Simplified type |
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.
| import type { ReactNode, ChangeEvent, CSSProperties } from "react"; | ||
| // Remove "../common/types" if it causes issues, otherwise keep it. | ||
| // Assuming TextInputTypes is simple string union for now to be safe. | ||
| export type TextInputType = "text" | "number" | "email" | "password" | "search" | "tel" | "url" | "date" | "datetime-local" | "time"; |
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.
Try using the common types first. If that doesn’t work, we can use a custom type here.
Thank You! @swayamk05 I’ve added some initial feedback please make sure to apply it consistently across all the changes in this PR. |
|
Thanks for the detailed review! I've updated the types to be strict (removed any), fixed the Headless UI deprecations, and added full descriptions to the Storybook argTypes. Everything should be good to go now. |
|
@swayamk05, could you please align the styles and subtypes of the components with the referenced Espresso by Frappe Figma design for the Select, TextInput, and TextArea components? For example, in TextArea, the underline and ghost variants are missing, and only sm, md, and lg sizes should be there. There are also different styles for various states such as success, warning, error, and loading. Also check other component styles as well. |
|
@b1ink0 Aligned Select, TextArea, and TextInput with the Espresso Design System. Added missing ghost/underline variants and success/warning/error states. |
|
|
||
| <div className="relative flex items-center"> | ||
| {prefix && ( | ||
| <div className={`absolute left-${iconPos} flex items-center text-gray-700 dark:text-gray-400 pointer-events-none`}> |
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.
Tailwind classes should never be dynamic, as these classes are generated at build time. You need to follow the same pattern used above conditionally applying the required classes using clsx. Please fix this throughout the changed files.
| // FIX: Added 'relative z-0' to define stacking context | ||
| // FIX: Kept 'resize-none' and dark placeholder colors | ||
| "w-full block outline-none appearance-none transition-colors resize-none placeholder:text-gray-800 dark:placeholder:text-gray-500 relative z-0", |
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.
Please remove these unnecessary comments. And also from throughout the changed files.
| export const AllOutline = () => ( | ||
| <div className="space-y-6 w-72 p-6 border rounded bg-white dark:bg-gray-900 dark:border-gray-700"> | ||
| <h3 className="text-sm font-bold text-gray-900 dark:text-white uppercase">Outline Variants</h3> | ||
| <Textarea variant="outline" placeholder="Default" /> | ||
| <Textarea variant="outline" state="success" placeholder="Success" value="Success State" /> | ||
| <Textarea variant="outline" state="warning" placeholder="Warning" value="Warning State" /> | ||
| <Textarea variant="outline" state="error" placeholder="Error" value="Error State" /> | ||
| </div> | ||
| ); |
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.
Don’t need to show all variants at the same time. It should display only the Outline variant by default, and use the Storybook args and render function pattern so users can change the variant from the Storybook controls panel.
We also don’t need the wrapper with a border around the components it doesn’t look correct for component demo in Storybook. Please look at how other components (like the Button component stories) handle this and apply the same approach.
Apply this feedback to all the stories added in this PR.
| const sizeClasses = { | ||
| sm: "text-sm h-7", | ||
| md: "text-base h-8", | ||
| lg: "text-lg h-10", | ||
| }[size as SizeKey]; |
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.
TextInput has one more size xl see: https://www.figma.com/design/pCdNO73AL3Zes5ltEL9PNU/Espresso-by-Frappe--Community-?node-id=8878-180431&t=YXozhg1vvvORVbBh-0
| // 4. UNDERLINE | ||
| if (variant === "underline") { | ||
| switch (state) { | ||
| case "error": | ||
| return "bg-transparent border-b border-red-500 text-red-800 rounded-none px-0 hover:border-red-600 focus:ring-0 focus:border-red-600 dark:text-red-400 dark:border-red-500"; | ||
| case "success": | ||
| return "bg-transparent border-b border-green-500 text-green-800 rounded-none px-0 hover:border-green-600 focus:ring-0 focus:border-green-600 dark:text-green-400 dark:border-green-500"; | ||
| case "warning": | ||
| return "bg-transparent border-b border-orange-500 text-orange-800 rounded-none px-0 hover:border-orange-600 focus:ring-0 focus:border-orange-600 dark:text-orange-400 dark:border-orange-500"; | ||
| default: | ||
| return "bg-transparent border-b border-gray-300 text-gray-900 rounded-none px-0 hover:border-gray-400 focus:ring-0 focus:border-gray-900 dark:text-white dark:border-gray-700 dark:hover:border-gray-500"; | ||
| } | ||
| } |
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.
Don’t use switch statements pattern. Follow the same pattern used for handling variants in the Button component and apply that approach consistently.
Please apply this feedback to all components added in this PR.
|
@swayamk05 Added more feedback! |
Description
Fixes #91
This PR implements three new form components (
Select,TextInput,Textarea) to align with the library's design system standards. These components are fully typed, interactive, and include comprehensive test coverage.Changes
1. Select Component
Listbox) to ensure full accessibility and keyboard navigation.size(sm, md, lg, xl) andvariant(subtle, outline, ghost) props.prefixicons and disabled options.2. TextInput Component
label,description, anderrormessage rendering.prefixandsuffixslots for icons/text.3. Textarea Component
rowscontrol along with standard form props (label,error,description).TextInputfor consistent UI.4. Quality Assurance
types.tsfiles.Verification
I have verified the changes locally:
npm run test:unitpassed successfully for all new components.npm run lint:jsandnpm run lint:typespassed.Checklist
main.npm run lint:js:fix).