Test and minor fixes for Accordion, Button, and Progress components#668
Test and minor fixes for Accordion, Button, and Progress components#668chromium-52 wants to merge 13 commits intorad-ui:mainfrom chromium-52:test-components
Conversation
WalkthroughThis pull request introduces comprehensive improvements to the Accordion component and related UI components. The changes focus on enhancing type safety, refactoring context management, and improving component rendering logic. Key modifications include updating prop types, introducing context type definitions, removing unnecessary props, and adding validation for React nodes. The changes also include new test suites for the Accordion and Progress components, ensuring robust functionality and error handling. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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 (
|
| @@ -0,0 +1,13 @@ | |||
| import React from 'react'; | |||
|
|
|||
| export const isReactNode = (value: any): value is React.ReactNode => { | |||
There was a problem hiding this comment.
what will this be used for? is it like a util function for us to use and check if its a React node?
consider leaving some comments explaining what it does and how it's expected to be used :)
There was a problem hiding this comment.
Yup sounds good! I think what the function does is fairly straightforward given its name and signature (this is also a somewhat common pattern) but its expected usage isn't and I'll document this (I just like to be a little careful with comments because code can change quickly and comments can become outdated :) )
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/components/ui/Accordion/contexts/AccordionItemContext.tsx (2)
1-1: Ignore false positive lint warning about 'FocusEvent'.
Although the static analysis flags 'FocusEvent' as unused, it's clearly referenced in line 6. Safe to disregard this particular lint warning.🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'FocusEvent' is defined but never used
11-17: Default no-op handlers may hide errors.
Empty arrow functions work as fallbacks, but they can make debugging harder if an expected handler is never set. Consider logging a warning or throwing an error in development to avoid silent failures.src/components/ui/Progress/fragments/ProgressRoot.tsx (1)
9-9: Ensure children are valid nodes if needed.
While typically safe, consider optionally leveraging the new isReactNode utility if there's a risk of receiving non-React content here.src/components/ui/Accordion/fragments/AccordionItem.tsx (3)
1-1: Consider verifying the linter’s warning about FocusEvent
Your usage of FocusEvent as a type parameter in handleBlurEvent should be recognized by the compiler. If the linter continues to complain about "defined but never used," you could consider aliasing FocusEvent to _FocusEvent or removing the import if it’s truly unnecessary.🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'FocusEvent' is defined but never used
13-14: Reassess necessity of 'null' in itemValue
Since 'value' is required as a number, retaining a 'null' possibility in state may be unnecessary unless there's a valid reason to reset itemValue to null. If so, consider documenting that case.
42-42: Event parameter is currently unused
handleBlurEvent doesn’t reference the event object. If the event isn’t needed, omit it or prefix it with an underscore to silence lint warnings and clarify intent.src/components/ui/Accordion/tests/Accordion.test.tsx (1)
5-5: AccordionProps lint warning likely a false positive
The type is used at line 7 for defining items. If the linter continues to flag it, consider referencing the type more directly (e.g., in a function signature) or disabling the misleading rule.🧰 Tools
🪛 GitHub Check: lint
[warning] 5-5:
'AccordionProps' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/ui/Accordion/Accordion.tsx(2 hunks)src/components/ui/Accordion/contexts/AccordionContext.tsx(1 hunks)src/components/ui/Accordion/contexts/AccordionItemContext.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionContent.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(4 hunks)src/components/ui/Accordion/fragments/AccordionTrigger.tsx(2 hunks)src/components/ui/Accordion/tests/Accordion.test.tsx(1 hunks)src/components/ui/Button/tests/Button.test.tsx(1 hunks)src/components/ui/Progress/Progress.tsx(1 hunks)src/components/ui/Progress/fragments/ProgressIndicator.tsx(1 hunks)src/components/ui/Progress/fragments/ProgressRoot.tsx(1 hunks)src/components/ui/Progress/tests/Progress.test.tsx(1 hunks)src/core/types.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/components/ui/Accordion/contexts/AccordionItemContext.tsx
[warning] 1-1:
'FocusEvent' is defined but never used
src/components/ui/Accordion/tests/Accordion.test.tsx
[warning] 5-5:
'AccordionProps' is defined but never used
src/components/ui/Accordion/fragments/AccordionItem.tsx
[warning] 1-1:
'FocusEvent' is defined but never used
🔇 Additional comments (47)
src/core/types.ts (1)
3-13: Well-structured type guard for ReactNode.
The recursive check for array elements is particularly handy. This robust approach ensures reliable detection of any valid React node. Good job!
src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1)
3-9: Concise but explicit context type definition.
Defining the context shape with distinct handler signatures is a strong move for maintainability and clarity.
src/components/ui/Accordion/contexts/AccordionContext.tsx (2)
3-11: Accurate, self-documenting type structure.
Separating these properties into a typed context fosters clarity in usage throughout the Accordion. Great for readability.
13-21: Set-based default values are a good fallback.
Your default context reduces the risk of runtime errors in unwrapped components. This also helps reveal places where no provider is used inadvertently.
src/components/ui/Progress/fragments/ProgressRoot.tsx (1)
2-2: Nice addition of PropsWithChildren.
Explicitly extending the props for children clarifies that this component can wrap nested elements.
src/components/ui/Progress/Progress.tsx (2)
1-1: Removing unused imports looks clean!
This change decreases import overhead.
8-8: Confirm removal of children support.
By removing the inheritance from PropsWithChildren, this component no longer supports children directly. Ensure that this change aligns with your design requirements before merging.
src/components/ui/Accordion/fragments/AccordionContent.tsx (4)
8-8: Props streamlined.
The removal of index-related props simplifies the API. Good move for making the component more focused.
11-11: Use of default className is consistent.
The default value for className helps ensure consistent styling.
19-19: Improved accessibility ID reference.
Using content-${itemValue} ensures uniqueness and better aligns with item context.
21-21: Aria label alignment with item value.
Binding the aria-labelledby to section-${itemValue} is a clean approach for accessibility, correlating labels with content seamlessly.
src/components/ui/Progress/fragments/ProgressIndicator.tsx (2)
3-3: Importing COMPONENT_NAME centralizes naming.
This avoids hardcoding the component name in multiple places.
8-10: Refined IndicatorProps.
The interface no longer extends the ProgressProps, keeping the contract simpler and more component-specific.
src/components/ui/Accordion/Accordion.tsx (5)
7-7: Utility import isReactNode.
This is a great approach for validating React nodes and preventing rendering issues.
10-10: Stricter type for items.
Enforcing {title: string, content: React.ReactNode} ensures strong prop validation.
19-19: Custom trigger child.
This refactor ensures the displayed title is either a valid React node or an error message, improving resilience.
20-20: Check for valid string.
Since title is typed as string, consider removing the isReactNode check or re-check the type. If it can truly be a React node, the type annotation may need to be broader.
24-24: Check for valid content node.
This fallback helps avoid runtime crashes if any non-React node is passed. Good defensive programming.
src/components/ui/Progress/tests/Progress.test.tsx (6)
1-2: Imports look good.
The import statements correctly bring in React and testing-library utilities. No issues here.
4-4: Importing Progress component is straightforward.
The relative path import seems correct and consistent with the folder structure.
6-10: Good basic render test.
Testing that the component renders with a role of “progressbar” ensures basic accessibility coverage.
12-18: Effective clamping checks.
These tests properly verify the clamping behavior for values outside 0–100. Very thorough approach.
20-23: Validation of inverted minValue/maxValue scenario.
This test confirms that the component gracefully defaults to 0 when minValue > maxValue, adhering to robust error handling.
25-31: Re-render coverage.
Verifying that the component updates aria-valuenow upon prop changes is essential for dynamic scenarios. Great job covering this.
src/components/ui/Button/tests/Button.test.tsx (8)
1-3: Imports are standard.
Everything is correctly set up for React and testing-library usage, plus userEvent for interactive testing.
5-5: Button import is consistent.
No issues with importing the Button component.
7-11: Basic rendering test.
Ensures the Button displays child content. Nicely confirms the presence of the expected text.
13-17: Type attribute test.
Verifies the ‘type’ attribute is correctly mapped on the button. Good coverage for form usage.
19-23: Accent color attribute coverage.
Testing the data-accent-color attribute is a nice detail for theming checks.
25-29: Variant styling verification.
Ensuring the correct variant class is rendered helps maintain UI consistency.
31-35: Size API test.
Confirms the data-size attribute is accepted and correctly applied to the Button.
37-45: Click handler test.
Good coverage ensuring user interaction triggers the click function exactly once.
src/components/ui/Accordion/fragments/AccordionTrigger.tsx (1)
Line range hint 11-49: Transparent and accessible approach using itemValue.
Switching from index-based controls to itemValue-based logic improves clarity and accessibility. The code correctly toggles active states, updates context, and sets the aria attributes. Make sure itemValue is always defined in upstream data.
If desired, you can confirm usage by scanning all references to itemValue to ensure it’s never undefined:
✅ Verification successful
Let me analyze the usage of itemValue further by checking how it's initialized and passed down.
itemValue is properly initialized and managed throughout the accordion components
The verification confirms that itemValue is well-implemented:
- It's initialized from a required
valueprop in AccordionItem (passed as index from Accordion) - It's properly typed as
number | nullin the context - It's consistently used in AccordionTrigger and AccordionContent for aria attributes
- The default context value safely handles the null case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for itemValue references for verification
rg -A 2 'itemValue'
Length of output: 3827
Script:
#!/bin/bash
# Check how value prop is passed to AccordionItem
ast-grep --pattern 'const AccordionItem: React.FC<$_> = ({ value, $$ }) => {'
# Check for AccordionItem usage to see how value is passed
rg -A 2 '<AccordionItem'
Length of output: 1456
src/components/ui/Accordion/fragments/AccordionRoot.tsx (6)
1-1: Removed unused imports.
The removal of useEffect and sticks to basic React hooks is tidy.
15-15: Explicit ref typing.
Using useRef<HTMLDivElement | null>(null) clarifies the type and helps prevent accidental misuse.
18-19: State type clarity.
Defining activeItem as number | null and focusItem as Element | null is a best practice, preventing inadvertent type mismatches.
22-25: Null checks on accordionRef.
Running these checks early is crucial to avoid runtime errors if the ref is detached. Excellent defensive coding.
37-40: Consistent null checks for focusPrevItem.
Good consistency with focusNextItem to ensure safe DOM interactions.
60-60: AccordionContext provider property usage.
All relevant props (activeItem, setActiveItem, focusNextItem, focusPrevItem, etc.) are correctly exposed, supporting robust consumption by child components.
Also applies to: 62-62
src/components/ui/Accordion/fragments/AccordionItem.tsx (1)
8-8: Ensure all consumers provide a valid numeric value
Now that 'value' is required and typed as number, confirm that any usage of AccordionItem in the codebase guarantees a valid number. Otherwise, consider adding a fallback or default value.
src/components/ui/Accordion/tests/Accordion.test.tsx (7)
1-4: Imports are appropriate for testing
All necessary libraries for rendering and user interaction are correctly included.
7-11: Comprehensive test data
Providing multiple items with varied content ensures robust coverage for expansions and toggles.
13-18: Empty items scenario test
Verifying that the accordion renders correctly with zero items helps ensure stability.
19-27: Initial hidden content check
Ensures that all panels start closed by default, aligning with typical accordion behavior.
29-40: Toggle behavior validation
Verifies correct show/hide functionality when clicking the same header twice, confirming proper accordion state management.
42-56: Single-open rule test
Checks that opening a new item closes the previously open one, confirming the intended "one open at a time" design.
58-72: Graceful handling of invalid props
The tests confirm that invalid headers/content produce warning messages, reinforcing the reliability of your prop validation logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/core/types.tsx (3)
3-10: Use consistent docstring format for clarity.While the doc comments are thorough, it's helpful to adopt a consistent docstring style (e.g., JSDoc/TSDoc) to standardize documentation across the codebase.
11-18: Question the choice of returningfalse.Returning
falsewhen encountering an invalid node may obscure logical flow, especially if other callers of this function expect a React node ornull. Consider returningnullor throwing a more descriptive error, so it’s clearer that rendering is intentionally halted.
23-33: Ensure array validation performance remains acceptable.Recursively validating large arrays of nodes might be expensive. Keep an eye on performance if the function is called frequently with large data structures. You may want to memoize or short-circuit checks when you encounter an invalid member.
src/components/ui/Progress/tests/Progress.test.tsx (1)
25-31: Home in on dynamic binding.These tests confirm that the component properly updates when props change, reflecting a reactive UI design. Consider adding random or boundary value updates to broaden coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Accordion/Accordion.tsx(2 hunks)src/components/ui/Accordion/tests/Accordion.test.tsx(1 hunks)src/components/ui/Button/tests/Button.test.tsx(1 hunks)src/components/ui/Progress/tests/Progress.test.tsx(1 hunks)src/core/types.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/Accordion/Accordion.tsx
- src/components/ui/Button/tests/Button.test.tsx
🧰 Additional context used
🪛 GitHub Check: lint
src/components/ui/Accordion/tests/Accordion.test.tsx
[warning] 5-5:
'AccordionProps' is defined but never used
🔇 Additional comments (9)
src/components/ui/Progress/tests/Progress.test.tsx (3)
6-10: Good coverage of the basic render test.
Tests for the initial rendering and accessibility attributes provide confidence. Including more negative path testing, such as missing or invalid props, could further bolster reliability.
12-19: Commendable test for value clamping.
Verifying that the component clamps the value to valid bounds ensures correctness, preventing out-of-range states. Great job!
20-23: Edge-case coverage is beneficial.
Testing scenarios where minValue > maxValue ensures the component handles unexpected configurations gracefully. This increases robustness.
src/components/ui/Accordion/tests/Accordion.test.tsx (6)
7-11: Types help clarify prop structure.
Using AccordionProps['items'] is a promising approach for ensuring type-safety. If the linter complains that AccordionProps is unused, that may be a false positive, so you can safely ignore it unless it blocks builds.
13-17: Empty items scenario is well-tested.
Validating that the accordion renders even with an empty array ensures no unexpected crashes or rendering issues.
19-27: Initial state checks add robustness.
Ensuring that all content is hidden initially aligns with an intuitive default behavior for accordions.
29-40: User interaction test is effective.
Simulating clicks on headers to toggle visibility confirms correct interactive behavior. Great coverage of state changes.
42-56: Multiple open sections scenario.
Testing how the accordion handles transitions between open headers is essential for multi-item UIs. This approach comprehensively validates user expectations.
58-78: Good error handling tests.
The checks for invalid types and missing properties effectively confirm that console errors are triggered and that the component adapts gracefully. This ensures robustness against improper usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/types.tsx (3)
9-9: Fix incorrect JSDoc return descriptionThe JSDoc states it returns "false" for invalid nodes, but the function actually returns
null.-* @returns value if it's a valid React Node and false otherwise +* @returns value if it's a valid React Node and null otherwise
11-11: Consider usingunknowninstead ofanyfor better type safetyUsing
anybypasses TypeScript's type checking. Usingunknownwould force explicit type checking while maintaining flexibility.-export const validateReactNode = (value: any, name?: string): React.ReactNode => { +export const validateReactNode = (value: unknown, name?: string): React.ReactNode => {
16-16: Improve error handling strategyUsing
console.errorin production code could leak implementation details. Consider:
- Using a proper logging service
- Making error handling configurable
- Adding error boundaries for React components
- console.error(`${name || value} is not a valid React node`); + if (process.env.NODE_ENV !== 'production') { + console.error(`${name || value} is not a valid React node`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/types.tsx(1 hunks)
🔇 Additional comments (2)
src/core/types.tsx (2)
1-1: LGTM!
The React import is necessary and correctly implemented.
23-33: Enhance isReactNode implementation for completeness and performance
The implementation could be improved in several ways:
- Add support for object type checking (valid ReactNode can be an object)
- Optimize performance by checking array last (most expensive operation)
- Add unit tests for various scenarios
const isReactNode = (value: any): value is React.ReactNode => {
return (
typeof value === 'string' ||
typeof value === 'number' ||
typeof value === 'boolean' ||
value === null ||
value === undefined ||
- React.isValidElement(value) ||
- (Array.isArray(value) && value.every(isReactNode))
+ React.isValidElement(value) ||
+ (typeof value === 'object' && !Array.isArray(value) && value !== null) ||
+ (Array.isArray(value) && value.every(isReactNode))
);
};Let's verify the test coverage:
|
@chromium-52 will you be working on this? seems to be getting stale 😅 |
|
@kotAPI oh sorry forgot to mention that this PR is ready for review 😅. I started resolving the conflicts but noticed that for the Accordion component, there are conflicts in the code but more importantly in the logic as well. Since the accordion accounts for most of the changes, I think it'll be better if I just close this PR and open a new one with just the changes for the Button and Progress components. Will do this today or tomorrow |
2 participants
Test and minor fixes for the following three components:
<Accordion />a. TypeScript fixes
b. Fix a bug when an item's
titleorcontentis invalid/missing (and add a new function that checks if the given value is aReact.ReactNode)<Button /><Progress />a. Remove unnecessary props
Summary by CodeRabbit
New Features
Accordioncomponent with improved type safety for item properties.Accordioncontexts.AccordionContentandAccordionTrigger, improving accessibility.AccordionandProgresscomponents.Bug Fixes
Accordioncomponent for invalid item types.Documentation
Progresscomponent to ensure correct functionality under various conditions.