Better Accordion #363
Better Accordion #363
Conversation
WalkthroughThe recent changes to the Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
- Coverage 94.64% 94.44% -0.20%
==========================================
Files 10 10
Lines 56 54 -2
Branches 9 9
==========================================
- Hits 53 51 -2
Misses 3 3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range and nitpick comments (2)
src/components/ui/KnowledgeBase.MD (1)
1-1: Consider adding a brief introduction.Adding a brief introduction at the beginning of the document can help users understand the purpose of the data attributes.
# Data attributes used in Rad UI This document describes the data attributes used in Rad UI components to facilitate easier detection of state behavior from the DOM.src/components/ui/Accordion/shards/AccordionTrigger.tsx (1)
Line range hint
28-42: Provide an explicit type for the button element.The default type of a button is
submit, which causes the submission of a form when placed inside aformelement. This is likely not the behavior that you want inside a React application.- <button + <button type="button"Tools
Biome
[error] 28-42: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/components/ui/Accordion/Accordion.stories.js (1 hunks)
- src/components/ui/Accordion/shards/AccordionItem.tsx (3 hunks)
- src/components/ui/Accordion/shards/AccordionRoot.tsx (3 hunks)
- src/components/ui/Accordion/shards/AccordionTrigger.tsx (3 hunks)
- src/components/ui/KnowledgeBase.MD (1 hunks)
- src/core/batches.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- src/components/ui/Accordion/Accordion.stories.js
Additional context used
LanguageTool
src/components/ui/KnowledgeBase.MD
[grammar] ~3-~3: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ...etch number of elements of this type of elements by passing a ref `data-rad-ui-focus-el...(TYPE_OF_PLURAL)
Biome
src/components/ui/Accordion/shards/AccordionTrigger.tsx
[error] 28-42: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
Additional comments not posted (6)
src/components/ui/Accordion/shards/AccordionRoot.tsx (1)
18-19: Consider adding comments for state variables.Adding comments to explain the purpose of the
activeItemandfocusItemstate variables can improve code readability.- const [activeItem, setActiveItem] = useState(null); // keeps track of the active item, stores the - const [focusItem, setFocusItem] = useState(null); // stores the id of the item that should be focused + // Keeps track of the active item + const [activeItem, setActiveItem] = useState(null); + // Stores the id of the item that should be focused + const [focusItem, setFocusItem] = useState(null);Likely invalid or redundant comment.
src/components/ui/Accordion/shards/AccordionItem.tsx (5)
1-1: ImportuseRefcorrectly.The addition of
useRefto the imports is correct and necessary for the new functionality.
13-13: InitializeaccordionItemRefcorrectly.The initialization of
accordionItemRefusinguseRefis correct and aligns with the new requirements.
15-15: IncludefocusIteminuseContextcorrectly.The addition of
focusItemto theuseContextcall is correct and necessary for the new functionality.
51-51: Provide new context values correctly.The
AccordionItemContext.Providercorrectly includes the newhandleBlurEventandhandleClickEventfunctions.
53-62: Enhancedivelement with new attributes.The
divelement is correctly enhanced withaccordionItemRef,data-rad-ui-batch-element, and conditionaldata-rad-ui-focus-elementattributes.
| export const getPrevBatchItem = (batches=[]) => { | ||
| const activeItem = getActiveBatchItem(batches); | ||
| // get the next item, return it if it is not the last item | ||
| const prevItem = activeItem?.previousElementSibling; | ||
| if (prevItem) { | ||
| return prevItem; | ||
| } | ||
|
|
||
| // if it is the last item, return the last item | ||
| return batches[0]; | ||
| }; |
There was a problem hiding this comment.
Consider using nullish coalescing operator.
The getPrevBatchItem function can be simplified by using the nullish coalescing operator (??) to handle the case where prevItem is null.
- export const getPrevBatchItem = (batches=[]) => {
- const activeItem = getActiveBatchItem(batches);
- // get the next item, return it if it is not the last item
- const prevItem = activeItem?.previousElementSibling;
- if (prevItem) {
- return prevItem;
- }
- // if it is the last item, return the last item
- return batches[0];
+ export const getPrevBatchItem = (batches: NodeList): Element | null => {
+ const activeItem = getActiveBatchItem(batches);
+ return activeItem?.previousElementSibling ?? batches[0];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPrevBatchItem = (batches=[]) => { | |
| const activeItem = getActiveBatchItem(batches); | |
| // get the next item, return it if it is not the last item | |
| const prevItem = activeItem?.previousElementSibling; | |
| if (prevItem) { | |
| return prevItem; | |
| } | |
| // if it is the last item, return the last item | |
| return batches[0]; | |
| }; | |
| export const getPrevBatchItem = (batches: NodeList): Element | null => { | |
| const activeItem = getActiveBatchItem(batches); | |
| return activeItem?.previousElementSibling ?? batches[0]; |
…d-ui/ui into pranay/accordion-better-focus
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
src/components/ui/Avatar/shards/AvatarImage.tsx (1)
Line range hint
1-34: Add meaningful alternative text for accessibility.Ensure the
altattribute provides a meaningful description to help users relying on screen readers understand the content's purpose.- <img src={src} alt={alt} onError={handleImageError} onLoad={handleImageLoaded} className={`${rootClass} ${className}`} {...props} /> + <img src={src} alt={alt || 'Avatar image'} onError={handleImageError} onLoad={handleImageLoaded} className={`${rootClass} ${className}`} {...props} />Tools
Biome
[error] 33-34: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
(lint/a11y/useAltText)
src/components/ui/ToggleGroup/shards/ToggleItem.tsx (2)
Line range hint
22-25: Remove the unnecessary else clause.The else clause can be omitted because the previous branches break early.
- if (type === 'single') { - if (isActive) { - toggleContext?.setActiveToggles([]); - return; - } else { - toggleContext?.setActiveToggles([value]); - return; - } - } + if (type === 'single') { + if (isActive) { + toggleContext?.setActiveToggles([]); + return; + } + toggleContext?.setActiveToggles([value]); + return; + }
Line range hint
41-44: Add keyboard event handlers for accessibility.Ensure actions triggered using mouse events have corresponding keyboard events to account for keyboard-only navigation.
- return <div - className={`${isActive?'bg-blue-600':''}`} onClick={() => { - handleToggleSelect(); - }}>{children}</div>; + return <div + className={`${isActive?'bg-blue-600':''}`} onClick={() => { + handleToggleSelect(); + }} onKeyUp={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleToggleSelect(); + } + }} tabIndex={0}>{children}</div>;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/components/tools/Popper/Popper.stories.js (1 hunks)
- src/components/ui/Accordion/shards/AccordionItem.tsx (3 hunks)
- src/components/ui/Accordion/shards/AccordionTrigger.tsx (2 hunks)
- src/components/ui/Avatar/shards/AvatarImage.tsx (1 hunks)
- src/components/ui/Callout/shards/CalloutRoot.tsx (1 hunks)
- src/components/ui/ToggleGroup/shards/ToggleItem.tsx (1 hunks)
- styles/themes/components/accordion.scss (1 hunks)
Files skipped from review due to trivial changes (2)
- src/components/tools/Popper/Popper.stories.js
- src/components/ui/Callout/shards/CalloutRoot.tsx
Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Accordion/shards/AccordionItem.tsx
Additional context used
Biome
src/components/ui/Avatar/shards/AvatarImage.tsx
[error] 33-34: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
(lint/a11y/useAltText)
src/components/ui/ToggleGroup/shards/ToggleItem.tsx
[error] 22-25: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 41-44: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
src/components/ui/Accordion/shards/AccordionTrigger.tsx
[error] 24-27: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 37-56: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
Additional comments not posted (1)
styles/themes/components/accordion.scss (1)
3-5: LGTM!The style changes enhance the visual design and user experience of the accordion component.
Also applies to: 10-14, 24-24, 31-33, 41-41
| } else if (activeItem !== itemValue) { | ||
| setActiveItem(itemValue); | ||
| handleClickEvent(); | ||
| } |
There was a problem hiding this comment.
Remove the unnecessary else clause.
The else clause can be omitted because the previous branches break early.
- if (activeItem === itemValue) {
- setActiveItem(null);
- return;
- } else if (activeItem !== itemValue) {
- setActiveItem(itemValue);
- handleClickEvent();
- }
+ if (activeItem === itemValue) {
+ setActiveItem(null);
+ return;
+ }
+ setActiveItem(itemValue);
+ handleClickEvent();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (activeItem !== itemValue) { | |
| setActiveItem(itemValue); | |
| handleClickEvent(); | |
| } | |
| if (activeItem === itemValue) { | |
| setActiveItem(null); | |
| return; | |
| } | |
| setActiveItem(itemValue); | |
| handleClickEvent(); |
Tools
Biome
[error] 24-27: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/components/tools/SandboxEditor/SandboxEditor.js (3 hunks)
- src/components/ui/Accordion/Accordion.stories.js (2 hunks)
- src/components/ui/Accordion/Accordion.tsx (1 hunks)
- src/components/ui/Accordion/shards/AccordionRoot.tsx (3 hunks)
- src/components/ui/Accordion/shards/AccordionTrigger.tsx (2 hunks)
- src/core/batches.ts (1 hunks)
- src/examples/Colors/ColorsTemplate.js (1 hunks)
- styles/themes/components/accordion.scss (1 hunks)
Files skipped from review due to trivial changes (1)
- src/components/ui/Accordion/Accordion.tsx
Files skipped from review as they are similar to previous changes (4)
- src/components/ui/Accordion/Accordion.stories.js
- src/components/ui/Accordion/shards/AccordionRoot.tsx
- src/core/batches.ts
- styles/themes/components/accordion.scss
Additional context used
Biome
src/components/ui/Accordion/shards/AccordionTrigger.tsx
[error] 24-27: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 37-56: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
src/components/tools/SandboxEditor/SandboxEditor.js
[error] 9-9: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
[error] 13-13: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
[error] 19-22: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
Additional comments not posted (9)
src/components/ui/Accordion/shards/AccordionTrigger.tsx (3)
1-1: Ensure consistent import spacing.The import statement should maintain consistent spacing for readability. This is a minor style issue but helps in maintaining code quality.
- import React, {useContext} from 'react'; + import React, { useContext } from 'react';
17-17: Context usage expanded to include additional handlers.The expansion of the context to include
handleBlurEvent,handleClickEvent, andhandleFocusEventaligns with the PR's objectives to enhance accessibility. Ensure that these handlers are implemented correctly in the context provider.
30-32: Focus event handler implementation.The focus handler directly calls
handleFocusEvent. Verify thathandleFocusEventis appropriately handling focus-related logic within the context.Verification successful
Focus event handler implementation.
The
handleFocusEventfunction callsfocusCurrentItem, which is defined as follows:const focusCurrentItem = () => { const elem = accordionItemRef?.current; // set `data-rad-ui-focus-element` we are making it active and focusing on this item if (elem) { elem.setAttribute('data-rad-ui-focus-element', ''); } };The
focusCurrentItemfunction sets adata-rad-ui-focus-elementattribute on the current accordion item element, indicating that it is appropriately handling focus-related logic.
src/components/ui/Accordion/shards/AccordionItem.tsx:
const handleFocusEvent = () => { focusCurrentItem(); };const focusCurrentItem = () => { const elem = accordionItemRef?.current; if (elem) { elem.setAttribute('data-rad-ui-focus-element', ''); } };Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of `handleFocusEvent` in the context provider. ast-grep --lang javascript --pattern $'AccordionItemContext.Provider { $$$ handleFocusEvent($_) { $$$ } $$$ }'Length of output: 628
Script:
#!/bin/bash # Verify the implementation of `handleFocusEvent` function in the codebase. ast-grep --lang javascript --pattern 'function handleFocusEvent($_) { $$$ }'Length of output: 80
Script:
#!/bin/bash # Search for all occurrences of `handleFocusEvent` in the codebase to identify its definition and usage. rg 'handleFocusEvent'Length of output: 542
Script:
#!/bin/bash # Extract the definition of `handleFocusEvent` from `AccordionItem.tsx`. ast-grep --lang javascript --pattern 'const handleFocusEvent = () => { $$$ }' src/components/ui/Accordion/shards/AccordionItem.tsxLength of output: 374
Script:
#!/bin/bash # Search for the definition of `focusCurrentItem` in the codebase. rg 'function focusCurrentItem' --context 5Length of output: 44
Script:
#!/bin/bash # Search for all occurrences of `focusCurrentItem` in the codebase to identify its definition and usage. rg 'focusCurrentItem'Length of output: 273
Script:
#!/bin/bash # Extract the definition of `focusCurrentItem` from `AccordionItem.tsx`. ast-grep --lang javascript --pattern 'const focusCurrentItem = () => { $$$ }' src/components/ui/Accordion/shards/AccordionItem.tsxLength of output: 805
src/components/tools/SandboxEditor/SandboxEditor.js (3)
45-45: Button onClick event handling.The button correctly toggles the dark mode state. Ensure that the icon switches correctly represent the state.
47-48: Dynamic rendering of color selectors.The use of
mapfor rendering color selectors is efficient. However, ensure thatkeyprops are unique and stable for better re-rendering performance.
2-2: Review import statement for unused imports.Ensure that all imported components are used within the file to avoid unnecessary overhead.
Verification successful
All imports are used within the
SandboxEditor.jsfile.The verification shows that all imported components (
React,useEffect,useState,Button,Separator, andcolors) are utilized in the file.
ReactuseEffectuseStateButtonSeparatorcolorsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for unused imports in SandboxEditor.js ast-grep --lang javascript --pattern $'import { $$$ $_ as $_ { $$$ } $$$ }' --file src/components/tools/SandboxEditor/SandboxEditor.jsLength of output: 341
Script:
#!/bin/bash # Step 1: List all imports from SandboxEditor.js imports=$(ast-grep --lang javascript --pattern 'import { $$_ } from "$_";' src/components/tools/SandboxEditor/SandboxEditor.js) # Step 2: Check if each import is used in the file for import in $imports; do if ! rg -q "$import" src/components/tools/SandboxEditor/SandboxEditor.js; then echo "Unused import: $import" fi doneLength of output: 130
Script:
#!/bin/bash # Step 1: List all imports from SandboxEditor.js using a more general pattern imports=$(ast-grep --lang javascript --pattern 'import $_ from "$_";' src/components/tools/SandboxEditor/SandboxEditor.js) # Step 2: Check if each import is used in the file for import in $imports; do if ! rg -q "$import" src/components/tools/SandboxEditor/SandboxEditor.js; then echo "Unused import: $import" fi doneLength of output: 125
Script:
#!/bin/bash # Step 1: List all imports from SandboxEditor.js using rg imports=$(rg '^import .* from ' src/components/tools/SandboxEditor/SandboxEditor.js) # Step 2: Extract imported items and check if each is used in the file echo "$imports" | while read -r line; do # Extract the imported item (e.g., React, useState, useEffect) items=$(echo "$line" | sed -n 's/import \(.*\) from .*/\1/p' | tr -d '{}') for item in $items; do # Check if the imported item is used in the file if ! rg -q "\b$item\b" src/components/tools/SandboxEditor/SandboxEditor.js; then echo "Unused import: $item" fi done doneLength of output: 1858
Script:
#!/bin/bash # Step 1: List all imports from SandboxEditor.js using rg imports=$(rg '^import .* from ' src/components/tools/SandboxEditor/SandboxEditor.js) # Step 2: Extract imported items and check if each is used in the file echo "$imports" | while read -r line; do # Extract the imported item (e.g., React, useState, useEffect) items=$(echo "$line" | sed -n 's/import \(.*\) from .*/\1/p' | tr -d '{} ' | tr ',' '\n') for item in $items; do # Check if the imported item is used in the file if ! rg -q "\b$item\b" src/components/tools/SandboxEditor/SandboxEditor.js; then echo "Unused import: $item" fi done doneLength of output: 1796
src/examples/Colors/ColorsTemplate.js (3)
1-9: Simplified component for displaying color bubbles.The
ColorBubblecomponent is efficiently using inline styles for dynamic sizing. Ensure that thecolorClassprop is correctly passed and utilized.
12-404: Structured color data management.The
colorsobject is well-organized, allowing easy addition or removal of color categories. This structure enhances maintainability.
407-419: Dynamic rendering of color groups.The use of
mapto render color groups and bubbles is efficient and correctly implemented. Ensure that keys used in the map are unique to prevent any potential re-render issues.
| onFocus={onFocusHandler} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'ArrowDown') { | ||
| // prevent scrolling when pressing arrow keys | ||
| e.preventDefault(); | ||
| focusNextItem(); | ||
| } | ||
| if (e.key === 'ArrowUp') { | ||
| // prevent scrolling when pressing arrow keys | ||
| e.preventDefault(); | ||
| focusPrevItem(); | ||
| } | ||
| }} | ||
| onClick={() => { | ||
| setActiveItem(itemValue); | ||
| }} | ||
| onClick={onClickHandler} | ||
| aria-expanded={activeItem === itemValue} |
There was a problem hiding this comment.
Add explicit button type to prevent form submission.
The button within the component should explicitly declare its type to avoid unintended form submissions when nested within a form.
- <button
+ <button type="button"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onBlur={handleBlurEvent} | |
| onFocus={onFocusHandler} | |
| onKeyDown={(e) => { | |
| if (e.key === 'ArrowDown') { | |
| // prevent scrolling when pressing arrow keys | |
| e.preventDefault(); | |
| focusNextItem(); | |
| } | |
| if (e.key === 'ArrowUp') { | |
| // prevent scrolling when pressing arrow keys | |
| e.preventDefault(); | |
| focusPrevItem(); | |
| } | |
| }} | |
| onClick={() => { | |
| setActiveItem(itemValue); | |
| }} | |
| onClick={onClickHandler} | |
| <button type="button" | |
| onBlur={handleBlurEvent} | |
| onFocus={onFocusHandler} | |
| onKeyDown={(e) => { | |
| if (e.key === 'ArrowDown') { | |
| // prevent scrolling when pressing arrow keys | |
| e.preventDefault(); | |
| focusNextItem(); | |
| } | |
| if (e.key === 'ArrowUp') { | |
| // prevent scrolling when pressing arrow keys | |
| e.preventDefault(); | |
| focusPrevItem(); | |
| } | |
| }} | |
| onClick={onClickHandler} |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/ui/Accordion/shards/AccordionTrigger.tsx (2 hunks)
Additional context used
Biome
src/components/ui/Accordion/shards/AccordionTrigger.tsx
[error] 24-27: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
Additional comments not posted (4)
src/components/ui/Accordion/shards/AccordionTrigger.tsx (4)
17-17: Consider adding a null check for context methods.The previous review comments suggest adding a null check for
handleBlurEventandhandleClickEventto ensure they are defined before calling them. This is a good practice to prevent runtime errors if the context does not provide these handlers.
20-27: Refactor conditional logic for clarity.The
else ifclause is unnecessary since the precedingifstatement returns early. Removing it simplifies the logic and aligns with the static analysis tool's hint.- } else if (activeItem !== itemValue) { + if (activeItem !== item Value) {Tools
Biome
[error] 24-27: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
38-54: Ensure button type is explicitly set to prevent form submissions.The button's type is explicitly set to "button" to prevent unintended form submissions when the button is inside a form. This is crucial for accessibility and to avoid unexpected behavior in web forms.
44-50: Good use of event prevention for navigation keys.Using
e.preventDefault()to stop the default action when arrow keys are pressed is a good practice, as it prevents the page from scrolling and allows for custom navigation logic to be executed.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/components/ui/Accordion/shards/AccordionItem.tsx (2 hunks)
- src/components/ui/Accordion/shards/AccordionRoot.tsx (3 hunks)
- src/components/ui/Accordion/shards/AccordionTrigger.tsx (2 hunks)
- styles/themes/components/accordion.scss (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/components/ui/Accordion/shards/AccordionRoot.tsx
- styles/themes/components/accordion.scss
Additional context used
Biome
src/components/ui/Accordion/shards/AccordionTrigger.tsx
[error] 23-26: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
src/components/ui/Accordion/shards/AccordionItem.tsx
[error] 17-17: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Additional comments not posted (11)
src/components/ui/Accordion/shards/AccordionTrigger.tsx (5)
1-1: Import statement update is necessary.The addition of
useContextis required for using context hooks in the component.
14-16: Context hooks are correctly utilized.The component correctly uses
useContextto access context values fromAccordionContextandAccordionItemContext.
29-31: Focus event handler is correctly implemented.The onFocusHandler function correctly calls the focus event handler.
37-53: Button type and event handlers are correctly implemented.The button type is set to "button" to prevent unintended form submissions, and event handlers are correctly assigned.
53-53: ARIA attributes are correctly implemented.The aria-expanded and aria-controls attributes are correctly used to enhance accessibility.
src/components/ui/Accordion/shards/AccordionItem.tsx (6)
1-1: Import statement update is necessary.The addition of
useId,useEffect, anduseRefis required for managing component state and lifecycle.
13-15: Refs and state management hooks are correctly utilized.The component correctly uses
useRefanduseStateto manage refs and state.
17-24: useEffect hook is correctly implemented.The useEffect hook correctly manages the open state of the accordion item based on the active item.
Tools
Biome
[error] 17-17: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
34-40: focusCurrentItem function is correctly implemented.The function correctly sets the
data-rad-ui-focus-elementattribute if the element is not null.
62-73: Context provider correctly includes new event handlers.The AccordionItemContext.Provider correctly includes the new event handlers.
27-32: Remove console.log statement in production code.The
console.logstatement should be removed in production code.- console.log('focus item', focusItemId);Likely invalid or redundant comment.
| const onClickHandler = () => { | ||
| if (activeItem === itemValue) { | ||
| setActiveItem(null); | ||
| return; | ||
| } else if (activeItem !== itemValue) { | ||
| setActiveItem(itemValue); | ||
| handleClickEvent(); | ||
| } |
There was a problem hiding this comment.
Refactor conditional logic for clarity.
The else clause can be omitted because the previous branches break early.
- } else if (activeItem !== itemValue) {
+ }
+ if (activeItem !== itemValue) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onClickHandler = () => { | |
| if (activeItem === itemValue) { | |
| setActiveItem(null); | |
| return; | |
| } else if (activeItem !== itemValue) { | |
| setActiveItem(itemValue); | |
| handleClickEvent(); | |
| } | |
| const onClickHandler = () => { | |
| if (activeItem === itemValue) { | |
| setActiveItem(null); | |
| return; | |
| } | |
| if (activeItem !== itemValue) { | |
| setActiveItem(itemValue); | |
| handleClickEvent(); | |
| } |
Tools
Biome
[error] 23-26: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
| const handleBlurEvent = (e) => { | ||
| // if clicked outside of the accordion, set activeItem to null | ||
| const elem = accordionItemRef?.current; | ||
|
|
||
| // remove `data-rad-ui-focus-element` attribute as we are not focusing on this item anymore | ||
| if (elem) { | ||
| elem.removeAttribute('data-rad-ui-focus-element'); | ||
| } | ||
| }; | ||
|
|
||
| const handleClickEvent = () => { | ||
| focusCurrentItem(); | ||
| }; | ||
|
|
||
| const handleFocusEvent = () => { | ||
| focusCurrentItem(); | ||
| }; |
There was a problem hiding this comment.
Ensure element is not null before accessing it in event handlers.
The event handlers should ensure that accordionItemRef.current is not null before accessing it.
const handleBlurEvent = (e) => {
// if clicked outside of the accordion, set activeItem to null
const elem = accordionItemRef?.current;
// remove `data-rad-ui-focus-element` attribute as we are not focusing on this item anymore
+ if (elem) {
elem.removeAttribute('data-rad-ui-focus-element');
+ }
};
const handleClickEvent = () => {
focusCurrentItem();
};
const handleFocusEvent = () => {
focusCurrentItem();
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleBlurEvent = (e) => { | |
| // if clicked outside of the accordion, set activeItem to null | |
| const elem = accordionItemRef?.current; | |
| // remove `data-rad-ui-focus-element` attribute as we are not focusing on this item anymore | |
| if (elem) { | |
| elem.removeAttribute('data-rad-ui-focus-element'); | |
| } | |
| }; | |
| const handleClickEvent = () => { | |
| focusCurrentItem(); | |
| }; | |
| const handleFocusEvent = () => { | |
| focusCurrentItem(); | |
| }; | |
| const handleBlurEvent = (e) => { | |
| // if clicked outside of the accordion, set activeItem to null | |
| const elem = accordionItemRef?.current; | |
| // remove `data-rad-ui-focus-element` attribute as we are not focusing on this item anymore | |
| if (elem) { | |
| elem.removeAttribute('data-rad-ui-focus-element'); | |
| } | |
| }; | |
| const handleClickEvent = () => { | |
| focusCurrentItem(); | |
| }; | |
| const handleFocusEvent = () => { | |
| focusCurrentItem(); | |
| }; |
This PR updates accordion component with better accessibility functionalities and low level helper APIs that add identifiers to DOM elements that makes detecting state behavior from dom easier.
This is definitely not the best way to do stuff, needs some refactoring and performance improvements, but the idea is well executed and in a better shape
Summary by CodeRabbit
New Features
Refactor
Style