feat(SideNavigationGroup): extend component, make it controllable #1513
Conversation
WalkthroughThe pull request enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Component
participant SNG as SideNavigationGroup
participant UA as useAnimations Hook
Parent->>SNG: Render with new props (isOpen, isMounted, etc.)
SNG->>UA: Initialize animations using listWrapperRef
UA-->>SNG: Return animation state (localIsOpen, localIsMounted, etc.)
SNG->>SNG: Render label, right-node based on props
SNG->>Parent: Trigger onClick handler on interaction
Assessment against linked issues
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
🪧 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 (
|
…ntrollable,export useAnimations
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/react-components/src/components/AppFrame/AppFrame.mdx: Language not supported
- packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.module.scss: Language not supported
- packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.tsx: Evaluated as low risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/react-components/src/components/AppFrame/components/SideNavigationGroup/types.ts (1)
27-51: Consider grouping related props into a sub-interface.The visibility control props (
isOpen,isMounted,setShouldBeVisible) could be grouped into a single interface for better organization and reusability.+interface IVisibilityControl { + isOpen?: IUseAnimations['isOpen']; + isMounted?: IUseAnimations['isMounted']; + setShouldBeVisible?: IUseAnimations['setShouldBeVisible']; +} -export interface ISideNavigationGroupProps extends ComponentCoreProps { +export interface ISideNavigationGroupProps extends ComponentCoreProps, IVisibilityControl { // ... other props - isOpen?: IUseAnimations['isOpen']; - isMounted?: IUseAnimations['isMounted']; - setShouldBeVisible?: IUseAnimations['setShouldBeVisible'];packages/react-components/src/index.ts (1)
3-3: Consider grouping hook exports.For better organization, consider grouping all hook exports together, similar to how component exports are grouped.
-export { useAnimations } from './hooks'; export * from './foundations'; +export * from './hooks'; // Export all hooks together export * from './providers';packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.tsx (3)
50-56: Consider memoizing derived values.The local state derivations could benefit from memoization to prevent unnecessary recalculations.
+const localValues = React.useMemo(() => ({ + isOpen: externalIsOpen ?? isOpen, + isMounted: externalIsMounted ?? isMounted, + rightNode: typeof rightNode === 'function' ? rightNode(localIsOpen) : rightNode, + label: typeof label === 'function' ? label(localIsOpen) : label +}), [externalIsOpen, isOpen, externalIsMounted, isMounted, rightNode, label, localIsOpen]);
71-73: Add safety check for non-element children.The current implementation assumes all children are React elements. Add a filter to handle potential non-element children.
-const listElements = React.Children.toArray(children) as React.ReactElement[]; +const listElements = React.Children.toArray(children) + .filter(React.isValidElement) as React.ReactElement[];
94-145: Consider extracting render methods.The conditional rendering logic could be simplified by extracting the label rendering into separate methods.
+const renderCollapsibleLabel = () => ( + <SideNavigationItem + leftNode={/* ... */} + label={/* ... */} + /* ... other props ... */ + /> +); +const renderSimpleLabel = () => ( + <span className={/* ... */}> + {/* ... label content ... */} + </span> +); -{isCollapsible || isLinkLabel ? ( - <SideNavigationItem /* ... */ /> -) : ( - <span /* ... */ /> -)} +{(isCollapsible || isLinkLabel) ? renderCollapsibleLabel() : renderSimpleLabel()}packages/react-components/src/components/AppFrame/AppFrame.mdx (1)
129-129: Add missing commas for better readability.The sentence structure needs improvement.
Apply this diff:
-Additionally, SideNavigationGroup exposes isOpen, isMounted and setShouldBeVisible props, allowing external control of the list visibility state. To maintain consistency with the default behavior of the component, when using these props it is recommended to do so with the useAnimations hook. For that purpose a ref can be created and passed as listWrapperRef prop to SideNavigationGroup - the same ref should also be passed to the external useAnimations hook. All of the aforementioned props should be used together to ensure SideNavigationGroup behaves as expected. +Additionally, SideNavigationGroup exposes isOpen, isMounted, and setShouldBeVisible props, allowing external control of the list visibility state. To maintain consistency with the default behavior of the component, when using these props, it is recommended to do so with the useAnimations hook. For that purpose, a ref can be created and passed as listWrapperRef prop to SideNavigationGroup - the same ref should also be passed to the external useAnimations hook. All the aforementioned props should be used together to ensure SideNavigationGroup behaves as expected.🧰 Tools
🪛 LanguageTool
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...vior of the component, when using these props it is recommended to do so with the `us...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~129-~129: Use a comma after an introductory phrase.
Context: ...to do so with theuseAnimationshook. For that purpose a ref can be created and passed as `lis...(COMMA_INTRODUCTORY_WORDS_PHRASES)
[style] ~129-~129: Consider removing “of” to be more concise
Context: ...d to the externaluseAnimationshook. All of the aforementioned props should be used tog...(ALL_OF_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-components/src/components/AppFrame/AppFrame.mdx(1 hunks)packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.module.scss(2 hunks)packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.spec.tsx(5 hunks)packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.tsx(3 hunks)packages/react-components/src/components/AppFrame/components/SideNavigationGroup/constants.ts(1 hunks)packages/react-components/src/components/AppFrame/components/SideNavigationGroup/types.ts(2 hunks)packages/react-components/src/hooks/useAnimations.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-components/src/components/AppFrame/components/SideNavigationGroup/constants.ts
🧰 Additional context used
🪛 LanguageTool
packages/react-components/src/components/AppFrame/AppFrame.mdx
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...vior of the component, when using these props it is recommended to do so with the `us...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~129-~129: Use a comma after an introductory phrase.
Context: ...to do so with the useAnimations hook. For that purpose a ref can be created and passed as `lis...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
[style] ~129-~129: Consider removing “of” to be more concise
Context: ...d to the external useAnimations hook. All of the aforementioned props should be used tog...
(ALL_OF_THE)
🔇 Additional comments (5)
packages/react-components/src/components/AppFrame/components/SideNavigationGroup/types.ts (2)
6-8: LGTM! Well-defined interface for onClick handler.
63-74: LGTM! Well-structured className props.The className props follow a consistent naming pattern and provide granular styling control.
packages/react-components/src/hooks/useAnimations.ts (1)
11-15: LGTM! Appropriate export of the interface.The export enables type reuse and maintains a single source of truth for animation-related types.
packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.spec.tsx (1)
29-222: LGTM! Comprehensive test coverage.The test suite thoroughly covers all new functionality including custom styling, link labels, right node rendering, click handlers, and visibility states.
packages/react-components/src/components/AppFrame/components/SideNavigationGroup/SideNavigationGroup.module.scss (1)
24-44: LGTM! Well-structured styling.The new classes follow BEM naming convention and implement proper flex layout patterns.
| @@ -126,6 +126,8 @@ import { SideNavigationGroup } from '@livechat/design-system-react-components'; | |||
| ... | |||
| ``` | |||
|
|
|||
| Additionally, `SideNavigationGroup` exposes `isOpen`, `isMounted` and `setShouldBeVisible` props, allowing external control of the list visibility state. To maintain consistency with the default behavior of the component, when using these props it is recommended to do so with the `useAnimations` hook. For that purpose a ref can be created and passed as `listWrapperRef` prop to `SideNavigationGroup` - the same ref should also be passed to the external `useAnimations` hook. All of the aforementioned props should be used together to ensure `SideNavigationGroup` behaves as expected. | |||
There was a problem hiding this comment.
Could we make this a little bit clearer? I found it a bit difficult to grasp at first. Maybe we could add something like: 'It allows parent components to have full control over the navigation group's state if needed' or something similar to help clarify its purpose
There was a problem hiding this comment.
Good point (turns out Cursor isn't exactly the best at helping with docs 🙄).
Would you say adding code example could be helpful here?
There was a problem hiding this comment.
That would be great 👍🏼
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-components/src/components/AppFrame/AppFrame.mdx (2)
106-106: Enhance clarity in collapsible behavior description.
Add a comma after "Alternatively," and insert "the" before "shouldOpenOnInitprop" to improve readability.🧰 Tools
🪛 LanguageTool
[misspelling] ~106-~106: Did you mean “site”?
Context: ...dren is the assumed pattern of building side navigation, however if you need to wrap...(SIDE_SITE)
[typographical] ~106-~106: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...ctiveto the child component yourself. Alternatively you can useshouldOpenOnInit` prop whi...(RB_LY_COMMA)
[uncategorized] ~106-~106: You might be missing the article “the” here.
Context: ...ent yourself. Alternatively you can useshouldOpenOnInitprop which will automa...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
129-131: Refine punctuation for control props explanation.
Place a comma after the introductory phrase for better clarity (e.g., "To maintain consistency with the default behavior ofSideNavigationGroup, when using these props, it is recommended to do so with theuseAnimationshook.").🧰 Tools
🪛 LanguageTool
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...SideNavigationGroup, when using these props it is recommended to do so with the `us...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~129-~129: Use a comma after an introductory phrase.
Context: ...to do so with theuseAnimationshook. For that purpose a ref can be created and passed as `lis...(COMMA_INTRODUCTORY_WORDS_PHRASES)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-components/src/components/AppFrame/AppFrame.mdx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/react-components/src/components/AppFrame/AppFrame.mdx
[misspelling] ~106-~106: Did you mean “site”?
Context: ...dren is the assumed pattern of building side navigation, however if you need to wrap...
(SIDE_SITE)
[typographical] ~106-~106: Consider adding a comma after ‘Alternatively’ for more clarity.
Context: ...ctiveto the child component yourself. Alternatively you can useshouldOpenOnInit` prop whi...
(RB_LY_COMMA)
[uncategorized] ~106-~106: You might be missing the article “the” here.
Context: ...ent yourself. Alternatively you can use shouldOpenOnInit prop which will automa...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~129-~129: Possible missing comma found.
Context: ...SideNavigationGroup, when using these props it is recommended to do so with the `us...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~129-~129: Use a comma after an introductory phrase.
Context: ...to do so with the useAnimations hook. For that purpose a ref can be created and passed as `lis...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
Resolves: #1498
Description
This PR adds missing customization options to
SideNavigationGroupcomponent, as well as prepares it for the future changes visible in the figma designs attached to the issue.Extended the
SideNavigationGroupcomponent:...classNameprops to make targeting elements with CSS more manageable,onClickprop, allowing for custom (and default) behavior when handling label click,rightNodefor non-collapsible case,isLinkLabel,isActiveprops - using them allows re-using visual styles already available in current component (and/orSideNavigationItemthat's being used as label)isOpen,isMounted,setShouldBeVisibleandlistWrapperRefprops,useAnimationshook to be used with above-mentioned props, to keep the animation style aligned,listElementsselecting fromchildrennodes (when single child element was present it wasn't an array, causinglistElements?.someto throw an error)Storybook
https://feature-1498--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Summary by CodeRabbit