New Component Added: Dropdown Menu #417
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the Dropdown component, transitioning from a monolithic implementation to a more modular, context-driven architecture. The changes include creating a new context for managing dropdown state, breaking down the component into smaller, reusable parts (Root, Trigger, Content), and updating related files like Popper and Button. The refactoring enhances component flexibility, improves type safety, and provides a more composable dropdown implementation. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)package.json (2)Line range hint The @floating-ui/react dependency is present which is essential for dropdown positioning. However, let's verify its integration: ✅ Verification successfulBased on the results, I can generate the final response as we have comprehensive evidence of @floating-ui/react integration: @floating-ui/react is properly integrated across components The dependency is well-integrated throughout the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash # Description: Verify floating-ui integration in Dropdown component # Check floating-ui imports in Dropdown component rg -A 3 "@floating-ui/react" # Look for floating-ui hook usage ast-grep --pattern 'useFloating()' Length of output: 5288
The new Dropdown component export is correctly added and follows the established pattern. Let's verify the build output path: ✅ Verification successfulExport path will be correctly generated during build The verification confirms that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash # Description: Verify the Dropdown component build output # Check if the source file exists fd -t f "Dropdown.tsx" src/ # Verify build script handles the new component rg "Dropdown" rollup*.config.js Length of output: 126 Script: #!/bin/bash # Let's verify the build configuration and component structure # Check the rollup config for build setup cat rollup-css.config.js rollup-tokens.config.js # Examine the main build config if it exists fd -t f "rollup.config.*" # Look at the component structure cat src/components/ui/Dropdown/Dropdown.tsx # Check if there are any build scripts in package.json rg "\"build\"" package.json Length of output: 1755 Script: #!/bin/bash # Let's check the main rollup config and build:components script # Check the main rollup config cat rollup.config.cjs # Check if there are any build:components scripts rg "\"build:components\"" package.json # Verify the component's directory structure ls -R src/components/ui/Dropdown/ Length of output: 2614 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: 2
Outside diff range and nitpick comments (1)
src/core/primitives/Button/index.tsx (1)
Line range hint
27-50: The changes improve type safety, ref integration, and accessibility. Well done!
- Explicitly typing the component's props using
ButtonPrimitivePropsimproves robustness by catching type mismatches.- Passing
buttonRefto the button element allows for better ref integration.- Setting aria attributes based on the provided props enhances accessibility by providing relevant information to assistive technologies.
- Providing a default
aria-descriptionfor disabled buttons improves the experience for assistive technology users.Consider restricting the props passed to the button element.
Currently, all props are passed through to the button element via
{...props}. While this ensures flexibility, it may allow unintended props to be passed. Consider creating a separate type for the props that should be allowed and use that to restrict the props passed to the button element. This can help prevent unintended behavior and improve the API of the component.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/components/tools/Popper/Popper.tsx (2 hunks)
- src/components/tools/SandboxEditor/SandboxEditor.tsx (1 hunks)
- src/components/ui/Button/Button.tsx (1 hunks)
- src/components/ui/Dropdown/Dropdown.stories.js (0 hunks)
- src/components/ui/Dropdown/Dropdown.stories.tsx (1 hunks)
- src/components/ui/Dropdown/Dropdown.tsx (1 hunks)
- src/core/primitives/Button/index.tsx (3 hunks)
Files not reviewed due to no reviewable changes (1)
- src/components/ui/Dropdown/Dropdown.stories.js
Files skipped from review due to trivial changes (1)
- src/components/tools/SandboxEditor/SandboxEditor.tsx
Additional comments not posted (11)
src/components/ui/Button/Button.tsx (3)
14-14: LGTM!The new optional
buttonRefprop is correctly typed and enhances the component's flexibility by allowing a reference to be passed to the underlying HTML button element. This change improves the component's usability for integration with other React components or libraries that require direct access to the DOM element.
17-17: LGTM!The
Buttoncomponent's function signature is correctly updated to include thebuttonRefprop, which is consistent with the changes made to theButtonPropstype. The prop is properly destructured from thepropsobject, and the rest of the function signature remains unchanged.
24-24: LGTM!The
buttonRefprop is correctly passed down to theButtonPrimitivecomponent, ensuring that the reference is properly forwarded to the underlying HTML button element. The prop is passed with the correct name and without any modifications, maintaining the expected behavior.src/components/ui/Dropdown/Dropdown.tsx (3)
1-3: LGTM!The imports are correct and necessary for the component.
12-16: LGTM!The new optional props added to the
DropdownPropsinterface enhance the flexibility and control over the dropdown's behavior. The types of the new props are correctly defined.
18-37: LGTM!The modifications to the
Dropdowncomponent enhance its usability and integration into various contexts. The use of theuseStateanduseEffecthooks ensures that the dropdown reflects the controlled state accurately. The conditional rendering based on thevisiblestate and the application of floating styles using theuseFloatinghook are implemented correctly.src/components/ui/Dropdown/Dropdown.stories.tsx (2)
17-27: LGTM!The
DefaultTriggerstory provides a clear example of theDropdowncomponent's default behavior. The use ofSandboxEditorandplaceholderWordsenhances the documentation and usability of the component.
29-42: LGTM!The
CustomTriggerstory effectively demonstrates the flexibility of theDropdowncomponent by utilizing a custom button as a trigger. The use ofuseStateandtoggleHiddenfunction provides a clear example of managing the dropdown's open state. ThebuttonRefprop is correctly passed to the custom button component, enhancing its functionality.src/core/primitives/Button/index.tsx (1)
20-24: Great job defining theButtonPrimitivePropstype!The type definition enhances type safety by explicitly specifying the expected properties for the
ButtonPrimitivecomponent. Makinglabelanddescriptionoptional provides flexibility, whilebuttonRefimproves integration with React's ref system. Using type intersection ensures the component can accept all standard button props and children.src/components/tools/Popper/Popper.tsx (2)
25-25: Improved type safety for theplacementproperty.The change from a limited set of string literals to the
Placementtype from@floating-ui/reactenhances type safety and allows for a broader range of placement options. This improves the component's flexibility and aligns with the PR objective.
26-26: Enhanced type inference for thechildrenproperty.Utilizing
PropsWithChildren['children']for thechildrenproperty provides better type inference and ensures compatibility with various React node structures. This change makes the component more robust and adaptable to different use cases.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/ui/Dropdown/Dropdown.tsx (1 hunks)
Additional comments not posted (3)
src/components/ui/Dropdown/Dropdown.tsx (3)
12-16: LGTM!The
DropdownPropsinterface has been correctly updated to include the new props:trigger,open,defaultOpen, andplacement. The prop types are accurately defined, enhancing the component's flexibility and usability.
18-23: LGTM!The
Dropdowncomponent function signature has been correctly updated to accept the new props. TheuseStateanduseEffecthooks are properly utilized to manage the dropdown's visibility state based on theopenprop, ensuring that the component accurately reflects the controlled state.
25-31: LGTM!The
useFloatinghook is correctly configured with theplacementprop to set the dropdown's position and thesizemiddleware to set the maximum height of the dropdown content. This ensures proper positioning and sizing of the dropdown.
| return <div> | ||
| | ||
| {trigger ? | ||
| trigger(refs.setReference) : | ||
| <ButtonPrimitive role='button' aria-expanded={visible} buttonRef={refs.setReference} onClick={() => setVisible((p) => !p)}>show/hide</ButtonPrimitive> | ||
| } | ||
| | ||
| <div style={{overflowY: 'auto', ...floatingStyles}} ref={refs.setFloating}> | ||
| {visible && children} | ||
| </div> | ||
| |
There was a problem hiding this comment.
Address the past review comment and update the aria-expanded attribute.
The rendering logic is correctly implemented and aligns with the component's new functionality. However, the past review comment suggesting to update the aria-expanded attribute to use the visible state is still valid and should be addressed to ensure accessibility.
Apply this diff to fix the issue:
-<ButtonPrimitive role='button' aria-expanded={open} buttonRef={refs.setReference} onClick={() => setVisible((p) => !p)}>show/hide</ButtonPrimitive> +<ButtonPrimitive role='button' aria-expanded={visible} buttonRef={refs.setReference} onClick={() => setVisible((p) => !p)}>show/hide</ButtonPrimitive>Committable suggestion was skipped due to low confidence.
| @@ -1,24 +1,46 @@ | |||
| import React from 'react'; | |||
| import Popper from '~/components/tools/Popper/Popper'; | |||
| import React, {PropsWithChildren, ReactElement, useEffect, useState} from 'react'; | |||
There was a problem hiding this comment.
can you refactor this so its composable? take a look at other components with shards folders to get a better idea
this is a good out of the box solution, but we should also provide a way to compose your own dropdowns if required
| can you add some screenshots on the PR? video would be great to demonstrate |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
src/components/ui/Dropdown/shards/DropdownRoot.tsx (1)
28-30: Simplify Context Value ObjectYou can simplify the context value by using ES6 shorthand notation for object properties.
Apply this diff to simplify the code:
return ( <DropdownContext.Provider - value={{visible: visible, toggleVisibility: () => setVisible((p) => !p), triggerRef: refs.setReference, portalRef: refs.setFloating, floatingPortalCss: floatingStyles}} + value={{ + visible, + toggleVisibility: () => setVisible((prev) => !prev), + triggerRef: refs.setReference, + portalRef: refs.setFloating, + floatingPortalCss: floatingStyles, + }} > {children} </DropdownContext.Provider> );
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/components/ui/Dropdown/Dropdown.stories.tsx (1 hunks)
- src/components/ui/Dropdown/Dropdown.tsx (1 hunks)
- src/components/ui/Dropdown/context/DropdownContext.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownPortal.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownRoot.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1 hunks)
Additional comments not posted (9)
src/components/ui/Dropdown/context/DropdownContext.tsx (1)
1-14: LGTM!The
DropdownContextis well-defined with a clear type definition and is properly initialized. The context provides a clean way to manage and share dropdown-related state and behavior across components. The type definition ensures type safety and helps catch potential type mismatches. The default value ofnullallows the context to be used in a provider to supply the actual values.Great job on creating this context! It will make the dropdown component more modular and easier to manage.
src/components/ui/Dropdown/Dropdown.tsx (4)
1-2: LGTM!The imports are correctly added to support the refactored
Dropdowncomponent.
4-8: The past review comment reminding to address the accessibility checklist item is still valid and applicable to the current code segment. Please ensure that the necessary accessibility attributes are added to align with the W3C WAI-ARIA Authoring Practices for the disclosure pattern.
10-14: Great refactor!The refactored
Dropdowncomponent structure aligns with the compositional approach suggested in the past review comment. The usage ofDropdownRootand its subcomponents enhances the modularity and flexibility of the dropdown, allowing for custom content through thechildrenprop.The changes align with the PR objectives and the AI-generated summary, transitioning the component to a more composable structure.
17-18: LGTM!Exporting the
DropdownRootas a static property of theDropdowncomponent is a good approach to enhance its modularity and reusability. This allows users to access and utilize theDropdownRootcomponent directly if needed.src/components/ui/Dropdown/shards/DropdownPortal.tsx (1)
1-17: LGTM!The
Portalcomponent is well-structured and modular. It effectively utilizes theDropdownContextto access and apply the necessary context values for dropdown visibility and styling.The component's logic is clear and concise:
- It checks for a null context and returns accordingly.
- It destructures the required values from the context.
- It renders a
divelement with the appropriate styling and ref.- It conditionally renders the children based on the
visiblestate.The component is reusable and flexible, as it accepts children as props and relies on the context values for its functionality.
Overall, the implementation is solid and adheres to best practices.
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1)
1-15: LGTM!The
Triggercomponent is well-designed and implemented:
- It correctly uses the React context to access the necessary state and functions.
- It properly handles the case when the context is null.
- The button is rendered with the correct role, aria attributes, and event handlers.
- The button displays the children prop or a default text.
- The component is modular and reusable, which is a good design practice.
- It correctly integrates with the existing context and components.
- The button is accessible, with proper aria attributes.
Great job!
src/components/ui/Dropdown/Dropdown.stories.tsx (1)
18-28: LGTM!The
DefaultTriggerstory correctly demonstrates the basic usage of theDropdowncomponent by passing theplaceholderWordsas children and wrapping it in aSandboxEditorfor interactive testing.src/components/ui/Dropdown/shards/DropdownRoot.tsx (1)
20-26: Ensure Middleware is Correctly Applied inuseFloatingThe
sizemiddleware'sapplyfunction should return a value or handle the resizing appropriately. Currently, it modifies themaxHeightbut doesn't account for other possible adjustments.Please verify the middleware behavior by checking the documentation or testing the component:
| export const CustomTrigger= () => { | ||
| return ( | ||
| <section> | ||
| <SandboxEditor> | ||
| <DropdownRoot> | ||
| {/* open={open} trigger={(ref) => <Button buttonRef={ref} onClick={toggleHidden}>Toggle</Button>} */} | ||
| <DropdownRoot.Trigger> | ||
| Toggle | ||
| </DropdownRoot.Trigger> | ||
| | ||
| <DropdownRoot.Portal> | ||
| {placeholderWords} | ||
| </DropdownRoot.Portal> | ||
| </DropdownRoot> | ||
| </SandboxEditor> | ||
| </section> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Consider using a Button component as the trigger for better user experience.
The current implementation of the CustomTrigger story uses a simple text element as the trigger, which may not provide the best user experience. Consider uncommenting the code block that uses a Button component as the trigger:
<DropdownRoot.Trigger> {(ref) => <Button buttonRef={ref} onClick={toggleHidden}>Toggle</Button>} </DropdownRoot.Trigger>This would provide a more interactive and visually appealing trigger for the dropdown.
| export const styled= () => { | ||
| return ( | ||
| <SandboxEditor> | ||
| <DropdownRoot> | ||
| {/* open={open} trigger={(ref) => <Button buttonRef={ref} onClick={toggleHidden}>Toggle</Button>} */} | ||
| <DropdownRoot.Trigger> | ||
| Click to show | ||
| </DropdownRoot.Trigger> | ||
| | ||
| <DropdownRoot.Portal> | ||
| {placeholderWords} | ||
| </DropdownRoot.Portal> | ||
| </DropdownRoot> | ||
| </SandboxEditor> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Consider using a Button component as the trigger for better user experience.
Similar to the CustomTrigger story, the current implementation of the styled story uses a simple text element as the trigger, which may not provide the best user experience. Consider uncommenting the code block that uses a Button component as the trigger:
<DropdownRoot.Trigger> {(ref) => <Button buttonRef={ref} onClick={toggleHidden}>Click to show</Button>} </DropdownRoot.Trigger>This would provide a more interactive and visually appealing trigger for the dropdown.
| export const DropdownRoot = ({children, open, defaultOpen = false, placement = 'bottom-start'}: DropdownProps) => { | ||
| const [visible, setVisible] = useState(defaultOpen); | ||
| | ||
| useEffect(() => { | ||
| open !== undefined && setVisible(open); | ||
| }, [open]); |
There was a problem hiding this comment.
Clarify Controlled vs. Uncontrolled Component Behavior
Mixing controlled and uncontrolled state can lead to unexpected behaviors. The DropdownRoot component accepts both open and defaultOpen props, which can cause conflicts in state management.
Consider updating the useEffect to handle changes more explicitly and prevent potential infinite loops:
useEffect(() => { - open !== undefined && setVisible(open); + if (open !== undefined) { + setVisible(open); + } }, [open]);Alternatively, you might want to enforce controlled behavior when the open prop is provided:
useEffect(() => { - if (open !== undefined) { - setVisible(open); - } + setVisible(open ?? defaultOpen); }, [open, defaultOpen]);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
src/components/ui/Dropdown/Dropdown.tsx (1)
1-12: Excellent progress on refactoring the Dropdown component!The changes demonstrate a significant improvement in the component's architecture, moving towards a more modular and composable design. This aligns well with the project's goals and addresses previous review comments. Great job!
Next steps to consider:
- Complete the accessibility implementation as per the WCAG guidelines.
- Add comprehensive documentation for the new component structure, including usage examples.
- Update or create new tests to cover the refactored component behavior.
- Consider adding TypeScript types for the exported components to improve developer experience.
src/core/primitives/Button/index.tsx (2)
20-24: LGTM! Consider adding JSDoc comments for better documentation.The new
ButtonPrimitivePropstype is well-defined and provides good type safety. It correctly extends the standard button attributes while adding custom props.Consider adding JSDoc comments to describe the purpose of each custom prop, especially
buttonRef. This would enhance the documentation and make it easier for other developers to understand and use the component. For example:export type ButtonPrimitiveProps = { /** Custom label for the button */ label?: string, /** Additional description for the button */ description?: string /** Ref to be forwarded to the underlying button element */ buttonRef?: React.LegacyRef<HTMLButtonElement> | undefined } & React.DetailedHTMLProps<React.ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement> & PropsWithChildren
50-50: LGTM! Consider destructuringbuttonRefwith other props.The
buttonRefprop is correctly used to forward the ref to the underlying button element. This is a good practice for reusable components.For consistency with the other destructured props, consider moving
buttonRefto the destructured object in the function parameters:const ButtonPrimitive = ({role='button', label='', description='', disabled=false, children, buttonRef, ...props}: ButtonPrimitiveProps) => { // ... rest of the component }This change would make the code more consistent and easier to read.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/components/ui/Dropdown/Dropdown.stories.tsx (1 hunks)
- src/components/ui/Dropdown/Dropdown.tsx (1 hunks)
- src/components/ui/Dropdown/context/DropdownContext.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownContent.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownRoot.tsx (1 hunks)
- src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1 hunks)
- src/core/primitives/Button/index.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/components/ui/Dropdown/Dropdown.stories.tsx
- src/components/ui/Dropdown/context/DropdownContext.tsx
- src/components/ui/Dropdown/shards/DropdownRoot.tsx
- src/components/ui/Dropdown/shards/DropdownTrigger.tsx
Additional comments not posted (8)
src/components/ui/Dropdown/Dropdown.tsx (3)
2-4: Excellent modular approach implemented!The new imports from the 'shards' folder indicate a shift towards a more composable and flexible Dropdown component structure. This change effectively addresses the previous review comment requesting a more composable design.
6-10: Expand the accessibility checklist for comprehensive implementation.It's great to see accessibility considerations included. However, the checklist is currently incomplete. To ensure full compliance with WCAG guidelines and improve user experience:
- Complete the checklist with all necessary ARIA attributes for the disclosure pattern.
- Include implementation details or code examples for each checklist item.
- Consider adding more accessibility features like keyboard navigation.
12-12: Great job implementing a composable Dropdown structure!The new export structure, providing separate Trigger, Content, and Root components, excellently addresses the previous request for a more composable design. This approach offers users the flexibility to create custom dropdown implementations while still providing the core building blocks.
src/components/ui/Dropdown/shards/DropdownContent.tsx (3)
1-2: LGTM: Imports are appropriate and well-structured.The imports are correctly defined and follow good practices by using named imports.
4-16: LGTM: Component structure follows React best practices.The
Contentcomponent is well-structured, using hooks appropriately and implementing conditional rendering based on context values.
11-15: LGTM: Rendering logic is sound and efficient.The component's rendering approach, including dynamic styling and conditional rendering of children, is well-implemented.
src/core/primitives/Button/index.tsx (2)
27-27: LGTM! Improved type safety withButtonPrimitiveProps.The updated component signature now uses the
ButtonPrimitivePropstype, which enhances type safety and code readability. The default values forrole,label,description, anddisabledare maintained, ensuring backwards compatibility.
Line range hint
1-58: Overall, great improvements to theButtonPrimitivecomponent!The changes in this file significantly enhance the
ButtonPrimitivecomponent:
- The new
ButtonPrimitivePropstype improves type safety and code readability.- The updated component signature uses the new type, maintaining backwards compatibility.
- The addition of the
buttonRefprop allows for proper ref forwarding.These improvements make the component more flexible and easier to use correctly. The changes follow React best practices and improve the overall quality of the codebase.
| const ctx = useContext(DropdownContext); | ||
| | ||
| if (ctx === null) return ctx; | ||
| | ||
| const {floatingContentCss, visible, floatingContentRef} = ctx; |
There was a problem hiding this comment.
Consider adding TypeScript types for improved type safety.
The context values are currently untyped. Adding proper TypeScript types would enhance code reliability and developer experience.
Consider defining an interface for the context and using it like this:
interface DropdownContextType { floatingContentCss: React.CSSProperties; visible: boolean; floatingContentRef: React.RefObject<HTMLDivElement>; } const ctx = useContext(DropdownContext) as DropdownContextType | null;| if (ctx === null) return ctx; | ||
| | ||
| const {floatingContentCss, visible, floatingContentRef} = ctx; |
There was a problem hiding this comment.
Consider using early return for improved readability.
The null check can be simplified using an early return, which can improve code readability.
Consider refactoring the null check like this:
if (ctx === null) return null; const {floatingContentCss, visible, floatingContentRef} = ctx;There was a problem hiding this comment.
Oh that's a nice catch.
There was a problem hiding this comment.
@decipher-cs, glad to hear that!
((^_^)/)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| return ( | ||
| <div style={{overflowY: 'auto', ...floatingContentCss}} ref={floatingContentRef}> | ||
| {visible && children} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Consider adding accessibility attributes for better user experience.
The dropdown content should have appropriate ARIA attributes to enhance accessibility.
Consider adding ARIA attributes to the content div:
<div style={{overflowY: 'auto', ...floatingContentCss}} ref={floatingContentRef} role="menu" aria-hidden={!visible} > {visible && children} </div>This change will improve the component's accessibility, making it more usable for people relying on screen readers.
| Here's a preview. dropdown.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
src/components/ui/Dropdown/Dropdown.stories.tsx (2)
2-5: Remove redundant imports and ensure consistent naming.There are redundant imports from the same path, and inconsistent naming between
DropdownandDropdownMenu. This could cause confusion and potential conflicts. Consider consolidating the imports and using consistent naming throughout the file.Apply this diff to fix the issue:
-import Dropdown from '~/components/ui/Dropdown/Dropdown'; -import {Meta} from '@storybook/react/*'; -import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; -import React from 'react'; -import DropdownMenu from '~/components/ui/Dropdown/Dropdown'; +import React from 'react'; +import { Meta } from '@storybook/react'; +import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; +import Dropdown from '~/components/ui/Dropdown/Dropdown';Ensure that you use
Dropdownconsistently in the rest of the file.
16-24: Update usage to reflect the renamed component 'Dropdown'.After renaming
DropdownMenutoDropdown, update the usage in this file to maintain consistency.Apply this diff to fix the usage:
-export const DefaultTrigger= () => { +export const DefaultTrigger = () => { return ( <section> <SandboxEditor> - <DropdownMenu> + <Dropdown> {placeholderWords} - </DropdownMenu> + </Dropdown> </SandboxEditor> </section> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/components/ui/Dropdown/Dropdown.stories.tsx(1 hunks)src/components/ui/Dropdown/Dropdown.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownContent.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownRoot.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownTrigger.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Dropdown/shards/DropdownRoot.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/ui/Dropdown/Dropdown.stories.tsx
[error] 46-48: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (2)
src/components/ui/Dropdown/Dropdown.tsx (1)
6-10: Reminder: Address the accessibility checklist item.
The comment on lines 6-10 mentions adding aria-control, but it appears incomplete. To enhance accessibility and comply with WAI-ARIA guidelines, please add the necessary aria-controls attribute to the Trigger component to link it to the Content component.
src/components/ui/Dropdown/shards/DropdownContent.tsx (1)
7-11: Add TypeScript types to the context for improved type safety.
Currently, the context values retrieved from ctx are untyped, which may lead to type safety issues. Consider defining an interface for the context and using it to ensure correctness.
Here's an example of how you might add types:
interface DropdownContextType { floatingContentCss: React.CSSProperties; visible: boolean; floatingContentRef: React.RefObject<HTMLDivElement>; } const ctx = useContext(DropdownContext) as DropdownContextType | null; There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
src/components/tools/SandboxEditor/SandboxEditor.tsx (5)
31-32: Remove extra blank lineMaintain consistent spacing by keeping only one blank line between sections.
- - +🧰 Tools
🪛 eslint
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
Line range hint
41-43: Remove empty useEffect hookThe empty useEffect hook with no dependencies and no implementation should be removed as it serves no purpose.
- useEffect(() => { - - }, []);🧰 Tools
🪛 eslint
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
28-30: Make ColorSelect dimensions configurableConsider making the dimensions configurable through props to improve component reusability.
-const dimensions = 32; +type ColorSelectProps = { + color: typeof colors[keyof typeof colors], + colorName: string, + changeAccentColor: (colorName: string) => void, + size?: number +} +const ColorSelect = ({ color, colorName, changeAccentColor, size = 32 }: ColorSelectProps) => { return <div onClick={() => changeAccentColor(colorName)} className='cursor-pointer rounded-full hover:border-gray-700 border' - style={{ width: dimensions, height: dimensions, backgroundColor: color.light['900'] }} + style={{ width: size, height: size, backgroundColor: color.light['900'] }} ></div>; };
Line range hint
63-63: Address @ts-ignore commentThe
@ts-ignorecomment suppresses TypeScript checks without explanation. Either:
- Fix the underlying type issue, or
- Document why the type check needs to be suppressed using
@ts-expect-errorwith an explanation🧰 Tools
🪛 eslint
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
Line range hint
47-95: Consider architectural improvements
- Consider using a CSS-in-JS solution or class name utility (like
clsxortailwind-merge) for better class name management and composition.- Consider wrapping the component with an error boundary to gracefully handle rendering errors.
🧰 Tools
🪛 eslint
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (2)
1-3: Fix spacing in imports for consistent code style.The imports have inconsistent spacing around destructured elements.
Apply this diff to fix the spacing:
-import React, {PropsWithChildren, useContext} from 'react'; -import DropdownContext from '../context/DropdownContext'; -import ButtonPrimitive, {ButtonPrimitiveProps} from '~/core/primitives/Button'; +import React, { PropsWithChildren, useContext } from 'react'; +import DropdownContext from '../context/DropdownContext'; +import ButtonPrimitive, { ButtonPrimitiveProps } from '~/core/primitives/Button';🧰 Tools
🪛 eslint
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
7-16: Consider adding type assertion for better type safety.The context usage could benefit from explicit typing to prevent potential runtime errors.
Consider updating the context usage like this:
-const ctx = useContext(DropdownContext); +const ctx = useContext(DropdownContext) as DropdownContextType | null; if (ctx === null) throw new Error('Component can only be used within a dropdown'); -const {visible, toggleVisibility, triggerRef} = ctx; +const { visible, toggleVisibility, triggerRef } = ctx;🧰 Tools
🪛 eslint
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/shards/DropdownContent.tsx (1)
14-16: Consider adding additional ARIA attributes for better accessibility.While the basic ARIA attributes are present, adding more context-specific attributes would improve accessibility.
Consider adding these attributes:
-<div role='menu' aria-hidden={!visible} style={{overflowY: 'auto', ...floatingContentCss}} ref={floatingContentRef} {...props}> +<div + role='menu' + aria-hidden={!visible} + aria-orientation="vertical" + aria-labelledby={props['aria-labelledby']} + style={{ overflowY: 'auto', ...floatingContentCss }} + ref={floatingContentRef} + {...props} +>🧰 Tools
🪛 eslint
[error] 14-14: A space is required after '{'.
(object-curly-spacing)
[error] 14-14: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/Dropdown.tsx (1)
28-30: Fix spacing around assignment operators.The assignments to Dropdown properties have inconsistent spacing.
Apply this diff to fix the spacing:
-Dropdown.Root= Root; -Dropdown.Trigger= Trigger; -Dropdown.Content= Content; +Dropdown.Root = Root; +Dropdown.Trigger = Trigger; +Dropdown.Content = Content;🧰 Tools
🪛 eslint
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Dropdown/shards/DropdownRoot.tsx (1)
5-10: Document prop types for better developer experienceThe prop types lack documentation which makes it harder for other developers to understand their purpose and usage.
export type DropdownRootProps = { + /** Controls the open state when used as a controlled component */ open?: boolean + /** Sets the initial open state when used as an uncontrolled component */ defaultOpen?: boolean + /** Determines the placement of the dropdown relative to its trigger */ placement?: Placement } & PropsWithChildrensrc/components/ui/Button/Button.tsx (1)
14-14: Document buttonRef prop usageThe new buttonRef prop lacks documentation about its purpose and typical use cases.
+ /** Reference to the underlying button element. Useful for integration with components like Dropdown */ buttonRef?: React.LegacyRef<HTMLButtonElement> | undefinedsrc/components/tools/Popper/Popper.tsx (1)
Line range hint
83-89: Clean up implementation details
- The
hoverDelayprop is defined but the delay configuration is commented out in the hover hook- There's a console.log in the click handler that should be removed
Apply these changes:
const hover = useHover(context, { - // delay: hoverDelay, + delay: hoverDelay, }); const { getReferenceProps, getFloatingProps } = useInteractions([ hover, role, dismiss ]);onClick: () => { - console.log('click'); }🧰 Tools
🪛 eslint
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 4-4: A space is required after '{'.
(object-curly-spacing)
[error] 4-4: A space is required before '}'.
(object-curly-spacing)
🪛 GitHub Check: lint
[warning] 4-4:
'autoPlacement' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/tools/Popper/Popper.tsx(2 hunks)src/components/tools/SandboxEditor/SandboxEditor.tsx(1 hunks)src/components/ui/Button/Button.tsx(1 hunks)src/components/ui/Dropdown/Dropdown.stories.tsx(1 hunks)src/components/ui/Dropdown/Dropdown.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownContent.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownRoot.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownTrigger.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/tools/SandboxEditor/SandboxEditor.tsx
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Dropdown/Dropdown.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 4-4: A space is required after '{'.
(object-curly-spacing)
[error] 4-4: A space is required before '}'.
(object-curly-spacing)
[error] 16-16: A space is required after '{'.
(object-curly-spacing)
[error] 16-16: A space is required before '}'.
(object-curly-spacing)
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
[error] 34-34: A space is required after '{'.
(object-curly-spacing)
[error] 34-34: A space is required before '}'.
(object-curly-spacing)
src/components/tools/Popper/Popper.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 4-4: A space is required after '{'.
(object-curly-spacing)
[error] 4-4: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Button/Button.tsx
[error] 17-17: A space is required after '{'.
(object-curly-spacing)
[error] 17-17: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/Dropdown.stories.tsx
[error] 1-1: '~/components/ui/Dropdown/Dropdown' imported multiple times.
(import/no-duplicates)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 5-5: '~/components/ui/Dropdown/Dropdown' imported multiple times.
(import/no-duplicates)
[error] 7-7: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: Unexpected trailing comma.
(comma-dangle)
[error] 16-16: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Dropdown/shards/DropdownContent.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 6-6: A space is required after '{'.
(object-curly-spacing)
[error] 6-6: A space is required before '}'.
(object-curly-spacing)
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 14-14: A space is required after '{'.
(object-curly-spacing)
[error] 14-14: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/shards/DropdownRoot.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 18-18: Operator '=' must be spaced.
(space-infix-ops)
[error] 18-18: Expected a line break after this opening brace.
(object-curly-newline)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: Object properties must go on a new line if they aren't all on the same line.
(object-property-newline)
[error] 20-20: A space is required after '{'.
(object-curly-spacing)
[error] 20-20: A space is required before '}'.
(object-curly-spacing)
[error] 21-21: Operator '+' must be spaced.
(space-infix-ops)
[error] 22-22: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
[error] 24-24: Expected a line break before this closing brace.
(object-curly-newline)
[error] 24-24: A space is required before '}'.
(object-curly-spacing)
[error] 26-26: A space is required after '{'.
(object-curly-spacing)
[error] 26-26: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
🪛 GitHub Check: lint
src/components/tools/Popper/Popper.tsx
[warning] 4-4:
'autoPlacement' is defined but never used
src/components/ui/Dropdown/Dropdown.stories.tsx
[failure] 1-1:
'~/components/ui/Dropdown/Dropdown' imported multiple times
[failure] 5-5:
'~/components/ui/Dropdown/Dropdown' imported multiple times
🔇 Additional comments (4)
src/components/tools/SandboxEditor/SandboxEditor.tsx (1)
32-32: LGTM! Type definition improvement
The simplified type definition className?: string is more idiomatic TypeScript, as:
- It correctly uses the optional property syntax
stringtype already includes empty string, makingstring | ''redundant
🧰 Tools
🪛 eslint
[error] 31-32: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Dropdown/shards/DropdownRoot.tsx (1)
14-16: 🛠️ Refactor suggestion
Improve controlled state management
The current implementation of the controlled state update can lead to unnecessary re-renders and potential race conditions.
useEffect(() => { - open !== undefined && setVisible(open); + if (open !== undefined) { + setVisible(open); + } }, [open]);Likely invalid or redundant comment.
src/components/tools/Popper/Popper.tsx (2)
24-25: LGTM! Type improvements enhance type safety
The changes to use Placement from floating-ui and PropsWithChildren improve type safety and maintainability.
6-7: Consider addressing TODOs before using in Dropdown component
The TODOs indicate potential issues:
- Portal usage could improve stacking context handling
- Collision detection issues might affect dropdown positioning
Since this component is being used in the new Dropdown implementation, these improvements would be valuable.
Let's check if these issues are documented:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1)
7-12: Enhance error message clarity.While the error handling is correct, the error message could be more specific about the required parent component.
- if (ctx === null) throw new Error('Component can only be used within a dropdown'); + if (ctx === null) throw new Error('DropdownTrigger must be used within a Dropdown.Root component');🧰 Tools
🪛 eslint
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/Dropdown.tsx (1)
28-30: Fix spacing around assignment operators.Add spaces around assignment operators for consistency.
-Dropdown.Root= Root; -Dropdown.Trigger= Trigger; -Dropdown.Content= Content; +Dropdown.Root = Root; +Dropdown.Trigger = Trigger; +Dropdown.Content = Content;🧰 Tools
🪛 eslint
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Dropdown/Dropdown.stories.tsx (1)
15-37: Add story descriptions for better documentation.Consider adding descriptions to stories using Storybook parameters to explain their purpose and usage.
export const DefaultTrigger = () => { + DefaultTrigger.parameters = { + docs: { + description: { + story: 'Basic usage of the Dropdown component with default trigger text.', + }, + }, + }; return (🧰 Tools
🪛 eslint
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 27-27: Operator '=' must be spaced.
(space-infix-ops)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Dropdown/Dropdown.stories.tsx(1 hunks)src/components/ui/Dropdown/Dropdown.tsx(1 hunks)src/components/ui/Dropdown/shards/DropdownTrigger.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/components/ui/Dropdown/Dropdown.stories.tsx (1)
Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.tsx:0-0 Timestamp: 2024-12-12T08:22:59.375Z Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`. src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1)
Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.tsx:0-0 Timestamp: 2024-12-12T08:22:59.375Z Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`. 🪛 eslint
src/components/ui/Dropdown/Dropdown.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 4-4: A space is required after '{'.
(object-curly-spacing)
[error] 4-4: A space is required before '}'.
(object-curly-spacing)
[error] 16-16: A space is required after '{'.
(object-curly-spacing)
[error] 16-16: A space is required before '}'.
(object-curly-spacing)
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
[error] 34-34: A space is required after '{'.
(object-curly-spacing)
[error] 34-34: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/Dropdown.stories.tsx
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 6-6: Operator '=' must be spaced.
(space-infix-ops)
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 27-27: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
🔇 Additional comments (4)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (2)
1-5: LGTM! Well-structured type definition.
The DropdownTriggerProps type correctly extends ButtonPrimitiveProps and provides flexibility with either children or content string.
🧰 Tools
🪛 eslint
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
14-16: LGTM! Well-implemented accessibility attributes.
The component correctly implements ARIA attributes and properly handles both content and children props.
src/components/ui/Dropdown/Dropdown.stories.tsx (2)
1-14: LGTM! Well-structured story setup.
The meta configuration and placeholder data are properly implemented with appropriate types and keys.
🧰 Tools
🪛 eslint
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 6-6: Operator '=' must be spaced.
(space-infix-ops)
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
55-65:
Add ARIA attributes to menu items.
The menu items need proper ARIA attributes for accessibility.
{[['Bookmark', '⌘+B'],['New Private Tab', '⌘+P'],['New Tab', '⌘+T'],['Close Tab', '⌘+W'],['Mute Tab', '⌘+M'], []].map(([name,shortcut]) => ( <div className="flex leading-8 px-4 py-1" key={name} + role="menuitem" + tabIndex={0} + aria-label={`${name}${shortcut ? ` (${shortcut})` : ''}`} >Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (4)
5-5: Consider making the props type more explicit.The current union type might lead to confusion as it allows both children and content props simultaneously. Consider using a discriminated union to make it mutually exclusive.
-export type DropdownTriggerProps = ButtonPrimitiveProps & (PropsWithChildren | {content: string}) +export type DropdownTriggerProps = ButtonPrimitiveProps & ({ + children: React.ReactNode; + content?: never; +} | { + children?: never; + content: string; +});
10-10: Enhance error message with component usage guidance.The error message could be more descriptive to help developers understand how to properly use the component.
- if (ctx === null) throw new Error('Component can only be used within a dropdown'); + if (ctx === null) throw new Error('Dropdown.Trigger must be used within a Dropdown.Root component. Please wrap it with Dropdown.Root.');
7-17: Consider memoizing the component for better performance.Since this component might be used in scenarios where frequent re-renders occur, consider memoizing it using React.memo.
-export default function Trigger({children, content, ...buttonProps}:DropdownTriggerProps) { +const Trigger = React.memo(function Trigger({children, content, ...buttonProps}:DropdownTriggerProps) { const ctx = useContext(DropdownContext); if (ctx === null) throw new Error('Dropdown.Trigger must be used within a Dropdown.Root component.'); const {visible, toggleVisibility, triggerRef} = ctx; return ( <ButtonPrimitive role='button' aria-label="dropdown menu toggle" aria-haspopup="true" aria-expanded={visible} buttonRef={triggerRef} onClick={toggleVisibility} {...buttonProps}>{children || content || 'Toggle'}</ButtonPrimitive> ); -} +}); + +export default Trigger;🧰 Tools
🪛 eslint
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
1-17: Fix object curly spacing throughout the file.The static analysis tools have flagged inconsistent spacing in object destructuring.
-import React, {PropsWithChildren, useContext} from 'react'; +import React, { PropsWithChildren, useContext } from 'react'; -import ButtonPrimitive, {ButtonPrimitiveProps} from '~/core/primitives/Button'; +import ButtonPrimitive, { ButtonPrimitiveProps } from '~/core/primitives/Button'; -export default function Trigger({children, content, ...buttonProps}:DropdownTriggerProps) { +export default function Trigger({ children, content, ...buttonProps }: DropdownTriggerProps) { - const {visible, toggleVisibility, triggerRef} = ctx; + const { visible, toggleVisibility, triggerRef } = ctx;🧰 Tools
🪛 eslint
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1)
Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.tsx:0-0 Timestamp: 2024-12-12T08:22:59.375Z Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`. 🪛 eslint
src/components/ui/Dropdown/shards/DropdownTrigger.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 7-7: A space is required after '{'.
(object-curly-spacing)
[error] 7-7: A space is required before '}'.
(object-curly-spacing)
[error] 12-12: A space is required after '{'.
(object-curly-spacing)
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
🔇 Additional comments (1)
src/components/ui/Dropdown/shards/DropdownTrigger.tsx (1)
14-16: LGTM! Good accessibility implementation.
The component implements proper ARIA attributes (role, aria-label, aria-haspopup, aria-expanded) which is excellent for accessibility. The fallback content strategy is also well thought out.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## main #417 +/- ## ======================================= Coverage 96.48% 96.48% ======================================= Files 17 17 Lines 199 199 Branches 41 41 ======================================= Hits 192 192 Misses 7 7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/components/tools/Popper/Popper.tsx (3)
25-26: Simplify children type definitionThe
Placementtype change is good as it provides better type safety and maintainability. However, the children type change is unnecessary.Revert the children type to use the standard React type:
placement?: Placement; - children?: PropsWithChildren['children']; + children?: React.ReactNode;Rationale:
React.ReactNodeis the standard type for children props and provides the same type safety.PropsWithChildrenis typically used as a type wrapper, not accessed directly.
7-8: Track TODOs in issue trackerThere are two important TODOs that should be tracked:
- Investigating Floating Portal usage
- Fixing collision detection issues
These could impact the dropdown functionality, especially in complex layouts.
Would you like me to create GitHub issues to track these improvements? This will help ensure they don't get lost and can be properly prioritized.
Line range hint
44-93: Consider extracting floating-ui logic into a custom hookThe component currently manages multiple floating-ui hooks and their interactions. This logic could be extracted into a custom hook like
usePopperto:
- Improve reusability across different floating components
- Make the component code more focused on rendering
- Simplify testing of positioning logic
Example structure:
const usePopper = (options: PopperOptions) => { // Current floating-ui hooks and logic return { refs, floatingStyles, isOpen, getReferenceProps, getFloatingProps }; };This would align well with the modular architecture being introduced in the dropdown refactoring.
🧰 Tools
🪛 eslint
[error] 5-5: A space is required after '{'.
(object-curly-spacing)
[error] 5-5: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Dropdown/stories/Dropdown.stories.tsx (2)
6-6: Improve demo content structure and accessibilityConsider these improvements for the placeholder content:
- Move the words array to a separate constants file
- Use semantic elements like
<li>within a<ul>for better accessibility-const placeholderWords= ['Eternity', 'Wisdom', ...].map((v) => (<div key={v}>{v}</div>)); +const placeholderWords = ['Eternity', 'Wisdom', ...].map((v) => ( + <li key={v} role="menuitem" className="dropdown-item"> + {v} + </li> +));🧰 Tools
🪛 eslint
[error] 6-6: Operator '=' must be spaced.
(space-infix-ops)
43-43: Reconsider defaultOpen in storySetting
defaultOpen={true}in the story might not be ideal as it doesn't represent the typical initial state of a dropdown menu.Consider removing
defaultOpenor creating a separate story specifically for demonstrating the open state.src/components/ui/Dropdown/Dropdown.tsx (3)
14-14: Make triggerContent more flexible by using ReactNode.The
triggerContentprop is currently limited to strings. Consider usingReactNodeto allow for more complex trigger content.-type DropdownProps = PropsWithChildren & {triggerContent?: string} +type DropdownProps = PropsWithChildren & {triggerContent?: React.ReactNode}
20-20: Remove unnecessary whitespace in JSX.There's an extra space in the Content component's opening tag.
- <Content > + <Content>
28-30: Fix spacing around assignment operators.The spacing around assignment operators should be consistent.
-Dropdown.Root= Root; -Dropdown.Trigger= Trigger; -Dropdown.Content= Content; +Dropdown.Root = Root; +Dropdown.Trigger = Trigger; +Dropdown.Content = Content;🧰 Tools
🪛 eslint
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/tools/Popper/Popper.tsx(2 hunks)src/components/ui/Dropdown/Dropdown.tsx(1 hunks)src/components/ui/Dropdown/stories/Dropdown.stories.js(0 hunks)src/components/ui/Dropdown/stories/Dropdown.stories.tsx(1 hunks)src/components/ui/Dropdown/tests/Dropdown.test.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/Dropdown/stories/Dropdown.stories.js
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/Dropdown/tests/Dropdown.test.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/ui/Dropdown/stories/Dropdown.stories.tsx (2)
Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.tsx:0-0 Timestamp: 2024-12-12T08:22:59.375Z Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`. Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50 Timestamp: 2024-12-12T08:34:33.079Z Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`. src/components/ui/Dropdown/Dropdown.tsx (1)
Learnt from: decipher-cs PR: rad-ui/ui#417 File: src/components/ui/Dropdown/Dropdown.tsx:0-0 Timestamp: 2024-12-12T08:22:59.375Z Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`. 🪛 eslint
src/components/ui/Dropdown/stories/Dropdown.stories.tsx
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 6-6: Operator '=' must be spaced.
(space-infix-ops)
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Operator '=' must be spaced.
(space-infix-ops)
[error] 27-27: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Dropdown/Dropdown.tsx
[error] 1-1: A space is required after '{'.
(object-curly-spacing)
[error] 1-1: A space is required before '}'.
(object-curly-spacing)
[error] 2-2: A space is required after '{'.
(object-curly-spacing)
[error] 2-2: A space is required before '}'.
(object-curly-spacing)
[error] 3-3: A space is required after '{'.
(object-curly-spacing)
[error] 3-3: A space is required before '}'.
(object-curly-spacing)
[error] 4-4: A space is required after '{'.
(object-curly-spacing)
[error] 4-4: A space is required before '}'.
(object-curly-spacing)
[error] 16-16: A space is required after '{'.
(object-curly-spacing)
[error] 16-16: A space is required before '}'.
(object-curly-spacing)
[error] 28-28: Operator '=' must be spaced.
(space-infix-ops)
[error] 29-29: Operator '=' must be spaced.
(space-infix-ops)
[error] 30-30: Operator '=' must be spaced.
(space-infix-ops)
[error] 34-34: A space is required after '{'.
(object-curly-spacing)
[error] 34-34: A space is required before '}'.
(object-curly-spacing)
src/components/tools/Popper/Popper.tsx
[error] 5-5: A space is required after '{'.
(object-curly-spacing)
[error] 5-5: A space is required before '}'.
(object-curly-spacing)
🔇 Additional comments (4)
src/components/tools/Popper/Popper.tsx (1)
1-1: Fix import formatting and remove unused import
The import statements need formatting fixes and cleanup:
Apply these changes:
-import React, { useState, useRef, PropsWithChildren } from 'react'; +import React, { useState, useRef, PropsWithChildren } from 'react'; -import {useFloating, useInteractions, useHover, FloatingArrow, arrow, offset, flip, autoPlacement, hide, shift, autoUpdate, useRole, useDismiss, Placement} from '@floating-ui/react'; +import { useFloating, useInteractions, useHover, FloatingArrow, arrow, offset, flip, hide, shift, autoUpdate, useRole, useDismiss, Placement } from '@floating-ui/react';Also applies to: 5-5
src/components/ui/Dropdown/stories/Dropdown.stories.tsx (1)
8-14: LGTM!
The Storybook metadata is well-structured and properly typed.
🧰 Tools
🪛 eslint
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Dropdown/Dropdown.tsx (2)
6-10:
Complete the WAI-ARIA implementation checklist.
The current checklist is incomplete. According to the W3C WAI-ARIA disclosure pattern, we need to implement:
aria-controls(already noted)aria-expandedaria-haspopup="menu"- Role attributes (
role="menu"for the dropdown androle="menuitem"for items)
Would you like me to provide a complete implementation of these accessibility features?
16-24: Verify consistent usage of the Dropdown component across the codebase.
Let's ensure the component is used consistently and that all required accessibility attributes are properly implemented where it's used.
✅ Verification successful
The initial search showed the component usage but we need to verify the accessibility implementation in the Trigger component itself and its usage patterns. The learnings also specifically mention checking existing ARIA attributes in the Trigger component, so let's examine those aspects more closely.
Let's try to get more information about the Trigger component implementation since the previous command had issues with the file type. We'll look at both the implementation and the complete file content.
Based on the output, I can now generate the final response since we have comprehensive information about the Dropdown component's implementation and usage. The search results show that the Dropdown's Trigger component already implements proper accessibility attributes including aria-label, aria-haspopup, and aria-expanded, and it's being used consistently across the codebase.
The Dropdown component implementation has proper accessibility attributes and consistent usage
The Trigger component already implements necessary ARIA attributes:
aria-label="dropdown menu toggle"aria-haspopup="true"aria-expanded={visible}
The component is being used consistently in the stories and maintains proper accessibility standards through its ButtonPrimitive base.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Dropdown component usage echo "Checking Dropdown component usage patterns..." rg -A 3 "<Dropdown[^>]*>" # Check for proper accessibility attributes echo "Checking accessibility attributes implementation..." rg -A 3 "aria-(controls|expanded|haspopup).*Dropdown" Length of output: 2398
Script:
#!/bin/bash # Let's check the Trigger component implementation for accessibility attributes echo "Checking Trigger component implementation..." ast-grep --pattern 'const Trigger = ({ $$ }) => { $$$ }' # Let's also check if there are any ARIA attributes in the Trigger component usage echo "Checking Trigger component usage with ARIA attributes..." rg -A 5 "<Trigger.*>" --type tsx Length of output: 345
Script:
#!/bin/bash # Let's check the Trigger component implementation echo "Checking Trigger component implementation..." ast-grep --pattern 'const Trigger = ({ $$ }) => { $$$ }' # Let's also check if there are any ARIA attributes in the Trigger component usage echo "Checking Trigger component usage with ARIA attributes..." rg -A 5 "<Trigger.*>" # Let's check the actual Trigger component file content echo "Checking Trigger component file content..." fd "Trigger.tsx" --exec cat {} Length of output: 6992
🧰 Tools
🪛 eslint
[error] 16-16: A space is required after '{'.
(object-curly-spacing)
[error] 16-16: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Button/Button.tsx Outdated
| customRootClass?: string; | ||
| variant?: 'solid' | 'outline' | 'soft' | 'ghost'; | ||
| size?: 'small' | 'medium' | 'large' | 'x-large'; | ||
| buttonRef?: React.LegacyRef<HTMLButtonElement> | undefined |
There was a problem hiding this comment.
with react 19, refs can be directly passed as props, I wonder how this will affect this
| @@ -0,0 +1,19 @@ | |||
| import React, {PropsWithChildren, useContext} from 'react'; | |||
There was a problem hiding this comment.
lets call the folder fragments instead of shards
| </svg> | ||
| </Dropdown.Trigger> | ||
| | ||
| <Dropdown.Content |
There was a problem hiding this comment.
Wont this have Dropdown.Item?
we also need to add portal support
| | ||
| // const placeholderWords= ['Eternity', 'Wisdom', 'Truth', 'Love', 'Freedom', 'Serenity', 'Hope', 'Courage', 'Grace', 'Harmony', 'Solitude', 'Enlightenment', 'Peace', 'Joy', 'Unity', 'Transcendence', 'Mystery', 'Compassion', 'Faith', 'Destiny'].map((v) => (<div key={v}>{v}</div>)); | ||
| | ||
| describe('Dropdown', () => { |
There was a problem hiding this comment.
we'll have to add tests for whatever features we support in this version,
so test for open, default open etc
There was a problem hiding this comment.
the more tests coverage we have - the better
ideally keyboard navigation for dropdown items should be covered
2 participants
buttonRefwhich passed the prop to down to the root<button/>through the button primitive.Summary by CodeRabbit
New Features
Content,Root, andTriggercomponents for enhanced dropdown functionality.Dropdowncomponent to showcase various configurations.buttonRefproperty to theButtoncomponent for better reference management.Bug Fixes
Chores
Dropdowncomponent.Dropdowncomponent.Dropdowncomponent.