Update jest config, support tests in TS, tests for customClassSwitcher #593
Conversation
WalkthroughThe pull request introduces several changes, including updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/core/primitives/Toggle/index.tsx (1)
12-12: LGTM! Consider documenting the default behavior.Making
onPressedChangeoptional with a default empty function improves API ergonomics and follows React patterns. The implementation correctly handles both controlled and uncontrolled scenarios.Consider adding a JSDoc comment to document that
onPressedChangedefaults to a no-op function when not provided:+ /** Callback fired when the toggle state changes. Defaults to no-op if not provided. */ onPressedChange?: (isPressed: boolean) => void;src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx (1)
Line range hint
6-120: Consider adding test for omitted onPressedChange.The test suite is comprehensive but could benefit from explicitly verifying that the component works correctly when
onPressedChangeis not provided.Add this test case:
it('works correctly without onPressedChange callback', () => { render(<TogglePrimitive>Test Content</TogglePrimitive>); const button = screen.getByRole('button'); // Verify that clicking works without errors expect(() => { fireEvent.click(button); }).not.toThrow(); expect(button).toHaveAttribute('data-state', 'on'); });src/core/customClassSwitcher/index.ts (3)
1-1: Consider exporting the prefix constantThe
RAD_UI_CLASS_PREFIXcould be useful in other parts of the codebase. Consider exporting it if other components need to construct class names with the same prefix.-const RAD_UI_CLASS_PREFIX = 'rad-ui'; +export const RAD_UI_CLASS_PREFIX = 'rad-ui';
3-7: Enhance JSDoc documentationWhile the current documentation explains the purpose, it could be more detailed with
@paramand@returnsannotations./** * Applies a custom root class the user provides, else applies the default rad-ui classes to the component * Rad UI's classes are based on this logic + * @param customRootClass - Custom class to override the default rad-ui class + * @param componentName - Name of the component to generate the default class name + * @returns The final class name to be applied to the component * */
16-17: Consider pre-compiling the regex patternFor better performance, especially if this function is called frequently, consider moving the regex pattern outside the function scope.
+const CAMEL_TO_KEBAB_REGEX = /([a-z0-9])([A-Z])/g; + export const customClassSwitcher = (customRootClass: string = '', componentName: string = ''): string => { // ... - const componentClassName = componentName.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase(); + const componentClassName = componentName.replace(CAMEL_TO_KEBAB_REGEX, '$1-$2').toLowerCase(); // ... };src/core/customClassSwitcher/customClassSwitcher.test.ts (1)
6-18: Enhance test coverage with additional edge casesWhile the current tests cover the main scenarios, consider adding tests for:
- Handling of undefined/null inputs
- Component names with multiple capital letters (e.g., "MyBigComponent")
- Component names with numbers (e.g., "Component2")
- Special characters in component names
it('handles undefined inputs gracefully', () => { expect(customClassSwitcher(undefined, undefined)).toBe(''); }); it('correctly formats complex component names', () => { expect(customClassSwitcher('', 'MyBigComponent')).toBe('rad-ui-my-big-component'); expect(customClassSwitcher('', 'Component2Test')).toBe('rad-ui-component2-test'); });package.json (1)
Line range hint
119-121: Consider pinning build tool versionsThe build tool dependencies have been updated to use caret versions:
rollup-plugin-postcssrollup-plugin-scsssassWhile allowing minor updates can bring bug fixes, consider pinning these versions to ensure consistent builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.gitignore(1 hunks)jest.config.cjs(0 hunks)jest.config.ts(1 hunks)package.json(3 hunks)src/components/ui/Avatar/tests/Avatar.test.tsx(0 hunks)src/components/ui/Card/Card.tsx(0 hunks)src/components/ui/Card/tests/Card.test.tsx(1 hunks)src/components/ui/Separator/tests/Separator.test.tsx(0 hunks)src/components/ui/Strong/tests/Strong.test.tsx(0 hunks)src/components/ui/Text/tests/Text.test.tsx(0 hunks)src/core/customClassSwitcher.ts(0 hunks)src/core/customClassSwitcher/customClassSwitcher.test.ts(1 hunks)src/core/customClassSwitcher/index.ts(1 hunks)src/core/primitives/Avatar/stories/AvatarPrimitive.stories.tsx(1 hunks)src/core/primitives/Primitive/tests/Primitive.test.tsx(1 hunks)src/core/primitives/Toggle/index.tsx(1 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (7)
- jest.config.cjs
- src/components/ui/Strong/tests/Strong.test.tsx
- src/components/ui/Avatar/tests/Avatar.test.tsx
- src/components/ui/Separator/tests/Separator.test.tsx
- src/core/customClassSwitcher.ts
- src/components/ui/Card/Card.tsx
- src/components/ui/Text/tests/Text.test.tsx
✅ Files skipped from review due to trivial changes (4)
- src/core/primitives/Primitive/tests/Primitive.test.tsx
- src/core/primitives/Avatar/stories/AvatarPrimitive.stories.tsx
- .gitignore
- jest.config.ts
🧰 Additional context used
📓 Learnings (2)
src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/Toggle/index.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🔇 Additional comments (7)
src/components/ui/Card/tests/Card.test.tsx (1)
Line range hint 1-31: LGTM! Well-structured test suite with clear, focused test cases.
The test suite follows good testing practices:
- Uses data-testid for reliable element selection
- Each test case focuses on a specific functionality
- Clear and descriptive test names
src/core/primitives/Toggle/tests/TogglePrimitive.test.tsx (1)
3-3: LGTM! Clean import path.
The simplified import path follows common conventions and improves readability.
src/core/customClassSwitcher/customClassSwitcher.test.ts (1)
1-1: Verify import path alias configuration
The import uses the ~ alias. Ensure this is properly configured in both TypeScript and Jest configurations.
✅ Verification successful
Path alias configuration is properly set up
The ~ alias is correctly configured in both TypeScript and Jest:
- TypeScript config maps
~/*tosrc/* - Jest config includes the corresponding mapping
^~/(.*)$to<rootDir>/src/$1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TypeScript and Jest configurations for path alias
echo "Checking TypeScript config..."
cat tsconfig.json | jq '.compilerOptions.paths'
echo "Checking Jest config..."
cat jest.config.ts
Length of output: 978
tsconfig.json (2)
19-20: LGTM: Types addition aligns with TypeScript test support
The addition of Jest and testing-library types is essential for TypeScript test support, which aligns perfectly with the PR objectives.
23-23: LGTM: Minor formatting improvement
The exclude array formatting change improves readability while maintaining the same functionality.
package.json (2)
77-77: LGTM: Test-related dependencies align with PR objectives
The addition of necessary TypeScript testing dependencies:
@babel/preset-typescript: For TypeScript compilation in tests@types/jest: For TypeScript type definitions in teststs-node: Required for running jest.config.ts
These changes directly support the PR's goal of TypeScript test support.
Also applies to: 95-95, 126-126
96-96: Verify React version compatibility with updated type definitions
The React-related updates introduce more flexible versioning:
@types/reactupdated to ^18.3.1reactandreact-domchanged to use caret versions
While this flexibility is generally good, we should ensure type definitions remain compatible with the React version range.
Also applies to: 131-133
✅ Verification successful
React types are compatible with the current version range
The version compatibility check shows:
- React versions: 18.2.0 to 18.3.1
- @types/react versions: 18.3.1 to 18.3.12
- No open breaking change issues found in DefinitelyTyped repository
The type definitions are maintained to be backward compatible within the same major version, and the minor version bump (18.3.x) of @types/react is safe to use with React 18.2.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if @types/react version is compatible with react version
npm info react@^18.2.0 version
npm info @types/react@^18.3.1 version
# Check for any reported compatibility issues
npm info @types/react@^18.3.1 repository.url | xargs -I {} gh api graphql -f query='
{
repository(owner: "DefinitelyTyped", name: "DefinitelyTyped") {
issues(first: 5, states: OPEN, labels: ["React", "Breaking Change"]) {
nodes {
title
url
}
}
}
}'
Length of output: 1356
| // tests | ||
|
|
||
| // test content | ||
| describe('Card', () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for the new customRootClass prop.
Based on the changes in Card.tsx, a new optional customRootClass prop was added. Please add test coverage to verify this new functionality.
Consider adding this test case:
test('renders Card component with customRootClass', () => {
render(<Card data-testid="card" customRootClass="custom-root" />);
expect(screen.getByTestId('card')).toHaveClass('custom-root');
});
|
I will fix CI/CD and re-request review if needed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/ui/Separator/Separator.tsx (1)
Line range hint
6-11: Consider removing redundant props field and improving type safetyThe
props?: anyfield is now redundant since the component extendsReact.ComponentProps<'div'>. Additionally, the type safety could be improved.Consider this refactoring:
export type SeparatorProps = { orientation?: 'horizontal' | 'vertical'; className?: string; customRootClass?: string; - props?: any; } & React.ComponentProps<'div'>;This change:
- Removes the redundant
propsfield as all div props are already included- Maintains clean and focused prop types
- Preserves type safety by relying on React's built-in types
src/components/ui/Strong/Strong.tsx (1)
Line range hint
12-16: Consider handling undefined classNameThe className concatenation could result in "undefined" appearing in the class string when className is not provided.
Consider this safer implementation:
-<strong className={`${rootClass} ${className}`} {...props} >{children}</strong> +<strong className={`${rootClass}${className ? ` ${className}` : ''}`} {...props}>{children}</strong>src/components/ui/Text/Text.tsx (2)
15-15: Great improvement in type safety!The change to extend
React.ComponentProps<'p'>is a significant improvement over the previousRecord<string, any>[]approach. It provides better type safety and IDE support for all valid HTML paragraph element props.Consider implementing the 'as' prop support (as mentioned in TODOs) using a polymorphic component pattern. This would allow the component to be rendered as different HTML elements while maintaining proper type safety. Example implementation:
type AsProp<C extends React.ElementType> = { as?: C; } & React.ComponentProps<C>; type TextProps<C extends React.ElementType = 'p'> = { children: React.ReactNode; customRootClass?: string; className?: string; } & AsProp<C>;
19-21: Consider defensive className handlingThe className concatenation looks good, but could benefit from a more defensive approach to handle undefined values and prevent extra spaces.
Consider using this more robust className handling:
- return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; + return <p className={[rootClass, className].filter(Boolean).join(' ')} {...props}>{children}</p>;This approach:
- Filters out falsy values (undefined, empty strings)
- Prevents extra spaces when either class is empty
- Is more maintainable if more class names need to be added in the future
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/ui/Card/Card.tsx(1 hunks)src/components/ui/Separator/Separator.tsx(1 hunks)src/components/ui/Separator/tests/Separator.test.tsx(0 hunks)src/components/ui/Strong/Strong.tsx(1 hunks)src/components/ui/Text/Text.tsx(1 hunks)src/core/primitives/Toggle/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/Separator/tests/Separator.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/Card/Card.tsx
- src/core/primitives/Toggle/index.tsx
🔇 Additional comments (4)
src/components/ui/Separator/Separator.tsx (1)
11-11: LGTM! Good practice extending React.ComponentProps
The extension of React.ComponentProps<'div'> enhances component flexibility while maintaining type safety. This change aligns well with similar updates in other components.
src/components/ui/Strong/Strong.tsx (2)
Line range hint 6-10: Great improvement in type safety!
The extension of React.ComponentProps<'strong'> is an excellent change that:
- Properly types all HTML strong element props
- Eliminates the unsafe
anytype - Maintains good TypeScript practices
Line range hint 14-14: Verify customClassSwitcher integration
Let's verify the integration with the new customClassSwitcher implementation mentioned in the summary.
✅ Verification successful
Let me generate one more verification script to check the actual implementation in the Strong component to ensure proper integration.
customClassSwitcher integration is properly implemented
The Strong component correctly implements the customClassSwitcher utility:
- Imports the utility from the core module
- Defines the COMPONENT_NAME constant
- Properly accepts customRootClass prop in the component props
- Correctly calls customClassSwitcher with both customRootClass and COMPONENT_NAME
- Follows the same pattern as other 23 components using this utility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new customClassSwitcher implementation and its usage
# Check the new customClassSwitcher implementation
echo "Checking customClassSwitcher implementation:"
ast-grep --pattern 'export const customClassSwitcher = $_'
# Check for any other components using customClassSwitcher
echo "Checking other components using customClassSwitcher:"
rg -l "customClassSwitcher.*COMPONENT_NAME" "src/components"
Length of output: 2377
Script:
#!/bin/bash
# Check the Strong component implementation
echo "Checking Strong component implementation:"
rg -A 10 "customClassSwitcher" "src/components/ui/Strong/Strong.tsx"
# Check if Strong component follows the same pattern as other components
echo "Checking Strong component exports and types:"
ast-grep --pattern 'export const Strong = ({ $$ }: StrongProps) => {
$$$
}'
Length of output: 901
src/components/ui/Text/Text.tsx (1)
15-21: Verify existing Text component usage
Since this is a core component change, let's verify that all existing usages are compatible with the new prop types.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Text component changes are safe to proceed
The verification shows all existing Text component usages are compatible with the new prop types:
- All instances only use standard props (className, children) which are maintained in the new implementation
- No usages of array-based props were found in the codebase
- The only props[] access patterns found are in Button component for aria attributes, unrelated to Text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all Text component usages
echo "Searching for Text component imports and usages..."
rg -l "import.*Text.*from.*components/ui/Text" | while read -r file; do
echo "=== $file ==="
# Show the context of Text component usage
rg -B 2 -A 2 "<Text" "$file"
done
# Look for potential props array usage that might break
echo "Checking for potential breaking changes..."
rg "props\[" -C 3
Length of output: 3651
|
Note: Why did this line cause the CI tests to run for so long?
|
2 participants
Developed as a part of #168
I’ve updated the Jest config to support and run tests written in TypeScript. I’ve also migrated all existing tests from JavaScript to TypeScript (though I only had to rename the files). I’m considering removing support for JavaScript tests entirely to enforce everyone writing tests in TypeScript, but this should be discussed.
Summary by CodeRabbit
New Features
asChildin theTogglePrimitivePropsinterface for enhanced flexibility.customClassSwitcherfunction to generate CSS class names.Textcomponent to validate its rendering and behavior.Bug Fixes
onPressedChangeinTogglePrimitivePropsto be optional, allowing for more flexible usage.Documentation
tsconfig.jsonto include Jest and Testing Library types.Tests