feat: enhance TextField component with additional className props#118
feat: enhance TextField component with additional className props#118lcmohsen wants to merge 1 commit into
Conversation
|
|
WalkthroughVersion bump for components package and updates to TextField props: label type narrowed to string; new optional className props added for group, prefix, and suffix; rendering updated to apply these classes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- Updated TextInputProps interface to include groupClassName, prefixClassName, and suffixClassName for better customization. - Modified TextField component to utilize new className props for FieldPrefix and FieldSuffix, improving styling flexibility.
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
lcmohsen
force-pushed
the
fix/pass-classnames-to-suffix-and-prefix-in-textfield
branch
from
7cd6a84 to
1d44933
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/components/src/ui/text-field.tsx (1)
103-114: Preserve consumer-supplied input className by merging props.className.The explicit className overrides any className provided via {...props}. Merge them to keep consumer overrides.
- className={cn('focus-visible:ring-0 focus-visible:ring-offset-0 border-input', { - 'rounded-l-none border-l-0': prefix, - 'rounded-r-none border-r-0': suffix, - })} + className={cn( + 'focus-visible:ring-0 focus-visible:ring-offset-0 border-input', + { + 'rounded-l-none border-l-0': prefix, + 'rounded-r-none border-r-0': suffix, + }, + props.className + )}
🧹 Nitpick comments (1)
packages/components/src/ui/text-field.tsx (1)
100-101: Nit: move always-true classes into the base string for clarity.These classes are unconditional; keeping them in the object literal with true adds noise.
- className={cn('flex group transition-all duration-200 rounded-md', { - 'field__input--with-prefix': prefix, - 'field__input--with-suffix': suffix, - 'focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-background': true - }, groupClassName)} + className={cn( + 'flex group transition-all duration-200 rounded-md focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-background', + { + 'field__input--with-prefix': prefix, + 'field__input--with-suffix': suffix, + }, + groupClassName + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/package.json(1 hunks)packages/components/src/ui/text-field.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
{package.json,packages/**/package.json,apps/**/package.json}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Use consistent versioning across packages
Files:
packages/components/package.json
{packages/**/package.json,apps/**/package.json}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Define peerDependencies, dependencies, and devDependencies appropriately in package.json
Files:
packages/components/package.json
packages/components/src/ui/**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration
Files:
packages/components/src/ui/text-field.tsx
packages/components/src/ui/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
Base UI components should be named as ComponentName in ui/ directory
Files:
packages/components/src/ui/text-field.tsx
**/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema
Files:
packages/components/src/ui/text-field.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
Always export both the component and its props type
Files:
packages/components/src/ui/text-field.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)
Files:
packages/components/src/ui/text-field.tsx
{apps,packages}/**/src/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')
Files:
packages/components/src/ui/text-field.tsx
packages/components/src/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports
Files:
packages/components/src/ui/text-field.tsx
{apps,packages}/**/src/**/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)
Files:
packages/components/src/ui/text-field.tsx
packages/components/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Don't import from remix-hook-form package in ui components
Files:
packages/components/src/ui/text-field.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Avoid circular dependencies
Files:
packages/components/src/ui/text-field.tsx
🧬 Code Graph Analysis (1)
packages/components/src/ui/text-field.tsx (2)
packages/components/src/ui/form.tsx (1)
FieldComponents(9-15)packages/components/src/ui/text-input.tsx (1)
InputProps(4-6)
🔇 Additional comments (4)
packages/components/package.json (1)
3-3: Manual verification needed: confirm TextField label usages and forms dependency pinsI wasn’t able to locate any
<TextField label={…}>instances passing JSX/ReactNode nor any pinned@lambdacurry/formsversions via automated search. Please manually verify:• That no consumers use non-string labels on
<TextField>(i.e. JSX or ReactNode).
• That all@lambdacurry/formspins in yourpackage.jsonfiles are as expected.If any JSX/ReactNode labels are in use, narrowing
TextField.labeltostringis a breaking change—either revert toReactNodeor bump to at least 0.20.0 (minor bump under 0.x). Otherwise, the current patch bump to 0.19.2 is sufficient.packages/components/src/ui/text-field.tsx (3)
65-67: New className props are well-scoped and consistent.groupClassName/prefixClassName/suffixClassName additions align with customization goals and preserve defaults.
103-103: Prefix className wiring looks good.
Passing prefixClassName through to FieldPrefix achieves the desired customization with no behavioral change.
115-115: Suffix className wiring looks good.
Same as prefix: non-breaking and enables targeted styling.
| control?: Control<FieldValues>; | ||
| name: FieldPath<FieldValues>; | ||
| label?: string | React.ReactNode; | ||
| label?: string; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid breaking change: keep label as ReactNode-compatible.
Narrowing label to string breaks valid use cases (icons, formatted labels, i18n components). Recommend restoring React.ReactNode.
Apply:
- label?: string;
+ label?: React.ReactNode;If you intend to keep it string-only, please confirm there are no callsites passing JSX. Use the script in package.json review to verify.
🏁 Script executed:
#!/bin/bash
set -e
echo "1) Finding TextField usages with non-string label props (JSX expressions):"
rg -n --type tsx '<TextField[^>]*label\s*=\s*\{' .
echo
echo "2) Finding any label prop passed as an expression anywhere in the repo:"
rg -n --type tsx 'label\s*=\s*\{' .Length of output: 248
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for TextField usages with label prop passed as an expression (non-string) in TSX/JSX files:"
rg -n "<TextField[^>]*label\s*=\s*\{" --glob "*.{tsx,jsx}" .Length of output: 284
Restore ReactNode for the label prop
Our search across all .tsx/.jsx files did not find any instances of non-string labels being passed to <TextField>, so reverting this change is non-breaking today and preserves future flexibility (icons, formatted labels, i18n components).
• File: packages/components/src/ui/text-field.tsx line 57
• Apply:
- label?: string;
+ label?: React.ReactNode;🤖 Prompt for AI Agents
In packages/components/src/ui/text-field.tsx around line 57, the label prop was
narrowed to string; restore it to allow ReactNode to preserve flexibility
(icons, formatted labels, i18n). Update the prop type from label?: string to
label?: React.ReactNode (or import ReactNode and use label?: ReactNode), ensure
React is imported or ReactNode is imported from 'react', and keep the prop
optional so existing usage remains valid.
| groupClassName, | ||
| prefixClassName, | ||
| suffixClassName, | ||
| ...props |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider forwarding refs properly instead of accepting a ref prop.
Function components don’t receive ref via props; won’t populate the ref variable unless you use React.forwardRef. This is orthogonal to this PR but worth addressing.
Example refactor (outside this hunk):
export const TextField = React.forwardRef<HTMLInputElement, TextInputProps>(function TextField(
{
control,
name,
label,
description,
className,
components,
prefix,
suffix,
groupClassName,
prefixClassName,
suffixClassName,
...props
},
ref
) {
// ... use `ref` here
});Then remove ref from TextInputProps entirely (it will be provided by forwardRef).
🤖 Prompt for AI Agents
In packages/components/src/ui/text-field.tsx around lines 80 to 83, the
component currently expects a ref prop via props which won’t be populated when
consumers use <TextField ref={...}>; change the component to use
React.forwardRef and accept (props, ref) so refs are forwarded correctly, remove
ref from the TextInputProps type, and update internal usage to call the
forwarded ref (or pass it to the underlying input element) instead of reading a
ref from props.
|
I'm not so sure adding this complexity is necessary, if we do need to do apply some styles to these we can use tailwind to target children, especially since we haven't needed to style these up until now. What do you think @jaruesink? |
3 participants
Summary by CodeRabbit