[WEB-4205] - Flyout Component for Meganav #625
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new navigation menu component using Radix UI's React Navigation Menu library. The changes include adding a new dependency to Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
aralovelace
force-pushed
the
WEB-4205_flyout
branch
2 times, most recently
from
36e72ad to
6b7cb89
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
src/core/Flyout.tsx (2)
13-26: Consider enhancing type safety with proper TypeScript interfaces.The component's props would benefit from being defined as a separate interface for better maintainability and documentation.
+interface MenuItem { + label: string; + content?: React.ReactNode; + link?: string; + panelStyling?: string; +} + +interface NavigationMenuDemoProps { + menuItems: MenuItem[]; + navMenuStyling?: string; + flyOutStyling?: string; +} + -const NavigationMenuDemo = ({ +const NavigationMenuDemo = ({ menuItems, navMenuStyling, flyOutStyling, -}: { - menuItems: { - label: string; - content?: React.ReactNode; - link?: string; - panelStyling?: string; - }[]; - navMenuStyling?: string; - flyOutStyling?: string; -}) => { +}: NavigationMenuDemoProps) => {
59-63: Add aria-label to improve accessibility.The navigation link should have an aria-label for better screen reader support.
- <NavigationMenuLink> + <NavigationMenuLink aria-label={menuItem.label}> <a href={menuItem.link} className={menuLinkStyles}> {menuItem.label} </a> </NavigationMenuLink>src/core/Flyout/Flyout.stories.tsx (1)
16-23: Consider moving sample data to a separate file.The product and platform arrays should be moved to a separate constants file for better maintainability and reusability.
+// src/core/Flyout/constants.ts +export const SAMPLE_PRODUCTS: ProductName[] = [ + "pubsub", + "chat", + "spaces", + "liveSync", + "assetTracking", + "liveObjects", +]; + +export const SAMPLE_PLATFORMS = [ + "Infrastructure", + "Integrations", + "SDKs", + "Security & Compliance", +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/core/Flyout/__snapshots__/Flyout.stories.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/core/Flyout.tsx(1 hunks)src/core/Flyout/Flyout.stories.tsx(1 hunks)tailwind.config.js(1 hunks)
🔇 Additional comments (2)
tailwind.config.js (1)
335-342: LGTM! Well-implemented animations.The new scale animations are well-defined and follow Tailwind's animation conventions. The timing and easing functions are appropriate for a navigation menu.
Also applies to: 347-348
package.json (1)
84-84: Verify package version compatibility and security.The added Radix UI Navigation Menu package version should be verified for compatibility with other Radix UI packages in the project.
✅ Verification successful
Version is compatible with other Radix UI packages
The added
@radix-ui/react-navigation-menu@^1.2.4is compatible with other Radix UI packages in the project as they all use the same major version (1.x.x). While a security audit couldn't be performed due to missing package-lock.json, the version is from a stable release of this well-maintained library.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check version compatibility with other Radix UI packages echo "Installed Radix UI package versions:" npm list | grep "@radix-ui" # Check for known vulnerabilities npm audit --json | jq '.advisories[] | select(.module_name | contains("@radix-ui/react-navigation-menu"))'Length of output: 4304
Script:
#!/bin/bash # Check if package-lock.json exists and get its content if [ -f "package-lock.json" ]; then echo "=== Checking package-lock.json ===" # Extract all @radix-ui dependencies and their resolved versions jq '.packages | to_entries[] | select(.key | contains("@radix-ui")) | {name: .key, version: .value.version}' package-lock.json # Run npm audit if package-lock.json exists npm audit --json | jq '.advisories[] | select(.module_name | contains("@radix-ui"))' else echo "package-lock.json not found" fiLength of output: 95
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
6b7cb89 to
6a57cc9
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
6a57cc9 to
1546bfc
Compare
There was a problem hiding this comment.
A great start, looking solid in Storybook already and you've made some good decisions with component structure. Left some comments mostly on code practice but below are some more product-oriented requests/suggestions ✏️
- Your props are set out well and make sense, but I would change the
...Stylingterminology to...ClassNameto be more consistent with other components and the React world in general (navMenuStyling can just beclassNameas it's the outer-most container) - what you have is fine, but it'd be really cool if we could get the left-to-right animations from the Radix example working, there's more on it in the advanced animation section of the Radix docs. This is, unless Jamie W has specifically told you not to do this
- we need a translucent underlay as well when the flyout is open, in the designs a 10% opacity neutral-1300 full screen box. Look into React portals to help you with this
- we need to ensure that the flyout aligns with container edges, as is requested in the design (pic below). Worth adding another story with the container inside a div with
ui-standard-containeron it (as the name suggests, our standard container) and making sure those alignments work
aralovelace
force-pushed
the
WEB-4205_flyout
branch
2 times, most recently
from
85f60ad to
b1a5833
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
b1a5833 to
ee3192a
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
91d067a to
8a6bbd2
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
6f011af to
b9f7c7c
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
1d3b5d7 to
809e7fa
Compare
aralovelace
force-pushed
the
WEB-4205_flyout
branch
from
809e7fa to
354b6d2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/core/Flyout.tsx (3)
62-67: Consider extracting default styles into constants.The default styles are defined inline. Moving them to a separate constants file would improve maintainability and reusability.
Create a new file
src/core/Flyout/constants.ts:export const DEFAULT_MENU_LINK_STYLING = "ui-text-menu3 font-bold text-neutral-1000 dark:neutral-300 hover:bg-neutral-100 dark:hover:bg-neutral-1200 hover:text-neutral-1300 dark:hover:text-neutral-000 px-12 py-8 flex items-center justify-between"; export const DEFAULT_VIEWPORT_STYLING = "relative overflow-hidden w-full h-[var(--radix-navigation-menu-viewport-height)] origin-[top_center] transition-[width,_height] duration-300 data-[state=closed]:animate-scale-out data-[state=open]:animate-scale-in sm:w-[var(--radix-navigation-menu-viewport-width)]"; export const PANEL_ANIMATION = "data-[motion=from-end]:animate-enter-from-right data-[motion=from-start]:animate-enter-from-left data-[motion=to-end]:animate-exit-to-right data-[motion=to-start]:animate-exit-to-left";
69-77: Add fade-out animation to FlyOverlay.The FlyOverlay component only has a fade-in animation. Adding a fade-out animation would improve the user experience.
- className={cn( - "absolute left-0 right-0 h-screen w-full opacity-0 animate-[fadeInTenPercent_200ms_ease]", - className, - )} + className={cn( + "absolute left-0 right-0 h-screen w-full opacity-0", + "data-[state=open]:animate-[fadeInTenPercent_200ms_ease]", + "data-[state=closed]:animate-[fadeOutTenPercent_200ms_ease]", + className, + )} + data-state={isOpen ? "open" : "closed"}Add the new animation to
tailwind.config.js:keyframes: { fadeOutTenPercent: { from: { opacity: 0.1 }, to: { opacity: 0 }, }, }, animation: { "fadeOutTenPercent": "fadeOutTenPercent 200ms ease", }
79-136: Consider accessibility improvements.The Flyout component could benefit from additional accessibility features.
Add ARIA labels and roles:
<NavigationMenu className={cn(className, "flex w-full")} onValueChange={(val) => setIsOpen(!!val)} + aria-label="Main navigation menu" > - <NavigationMenuList className="flex list-none center"> + <NavigationMenuList className="flex list-none center" role="menubar"> {menuItems.map(({ label, content, link, panelStyling }) => content ? ( - <NavigationMenuItem key={label}> + <NavigationMenuItem key={label} role="menuitem"> <NavigationMenuTrigger className={cn( "group outline-none focus:outline-none select-none cursor-pointer relative", DEFAULT_MENU_LINK_STYLING, menuLinkClassName, )} + aria-expanded="false" > {label} </NavigationMenuTrigger>src/core/Flyout/Flyout.stories.tsx (2)
22-38: Consider adding keyboard navigation to ProductsGrid.The ProductsGrid component handles mouse clicks but lacks keyboard navigation support.
Add keyboard navigation:
const ProductsGrid = () => { const [selectedProduct, setSelectedProduct] = useState<ProductName | null>( null, ); return ( <div className="grid grid-cols-2"> {Object.keys(products).map((product) => ( <ProductTile name={product as ProductName} key={product} selected={selectedProduct === product} onClick={() => setSelectedProduct(product as ProductName)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + setSelectedProduct(product as ProductName); + } + }} + tabIndex={0} + role="button" + aria-pressed={selectedProduct === product} /> ))} </div> ); };
85-124: Add type safety to menuItems.The menuItems array could benefit from stronger type safety.
Add a type definition:
type MenuItem = { label: string; content?: React.ReactNode; link?: string; panelStyling?: string; }; const menuItems: MenuItem[] = [ { label: "Home", content: null, link: "" }, { label: "Products", content: <Panels panelLeft={<ProductsGrid />} platforms={platforms} />, panelStyling: panelStyling, }, // ... ];tailwind.config.js (1)
335-358: Follow kebab-case naming convention for animations.The animation names should follow the kebab-case convention used by other animations in the file.
- "scale-in": { + "scale-in-menu": { from: { opacity: "0", transform: "rotateX(-10deg) scale(0.9)" }, to: { opacity: "1", transform: "rotateX(0deg) scale(1)" }, }, - "scale-out": { + "scale-out-menu": { from: { opacity: "1", transform: "rotateX(0deg) scale(1)" }, to: { opacity: "0", transform: "rotateX(-10deg) scale(0.95)" }, }, - "enter-from-right": { + "enter-from-right-menu": { from: { opacity: "0", transform: "translateX(200px)" }, to: { opacity: "1", transform: "translateX(0)" }, }, - "enter-from-left": { + "enter-from-left-menu": { from: { opacity: "0", transform: "translateX(-200px)" }, to: { opacity: "1", transform: "translateX(0)" }, }, - "exit-to-right": { + "exit-to-right-menu": { from: { opacity: "1", transform: "translateX(0)" }, to: { opacity: "0", transform: "translateX(200px)" }, }, - "exit-to-left": { + "exit-to-left-menu": { from: { opacity: "1", transform: "translateX(0)" }, to: { opacity: "0", transform: "translateX(-200px)" }, },Update the animation references:
animation: { - "scale-in": "scale-in 200ms ease", - "scale-out": "scale-out 200ms ease", - "enter-from-left": "enter-from-left 250ms ease", - "enter-from-right": "enter-from-right 250ms ease", - "exit-to-left": "exit-to-left 250ms ease", - "exit-to-right": "exit-to-right 250ms ease", + "scale-in-menu": "scale-in-menu 200ms ease", + "scale-out-menu": "scale-out-menu 200ms ease", + "enter-from-left-menu": "enter-from-left-menu 250ms ease", + "enter-from-right-menu": "enter-from-right-menu 250ms ease", + "exit-to-left-menu": "exit-to-left-menu 250ms ease", + "exit-to-right-menu": "exit-to-right-menu 250ms ease", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/core/Flyout/__snapshots__/Flyout.stories.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/core/Flyout.tsx(1 hunks)src/core/Flyout/Flyout.stories.tsx(1 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run linters and tests
🔇 Additional comments (1)
src/core/Flyout.tsx (1)
17-60: Props type definition looks good.The props are well-documented with JSDoc comments, making the component's API clear and easy to understand.
|
@jamiehenson thanks for the review and very accurate recommendation which is very helpful for me. I have updated and let me know your thoughts. thank you |
There was a problem hiding this comment.
@aralovelace this looks pretty solid, thanks for taking the feedback in.
In the interests of moving this forward, I've advanced this with a commit that does a couple more things:
- adds a fade-out transition to the flyout underlay on exit by deferring the unmounting of it by the length of the animation - a fairly useful trick for things like this. This has also been added to
Headerfor consistency - added a
delayDurationprop toNavigationMenuof 0 to remove the delay when hovering over a header item (we'd want it to be instant) - consolidated
FlyOverlayinsideFlyoutas we'd want them to be paired together, correct me if I'm wrong but I don't see a situation where we'd use one without the other
Let me know if you have thoughts or questions about any of the above.
There's still the matter of constraining the Flyout positioning to the container - we have a container story here but I'm not sure what functionality it demonstrates. I'm not 100% on what we actually need here - it's worth checking with Jamie W about how we want to align the flyout with the page content (with the container, or with the browser window with an offset?)
Also, don't forget to bump the package.json version before you merge.
| /** | ||
| * Optional styling for the flyout panel. | ||
| */ | ||
| panelStyling?: string; |
There was a problem hiding this comment.
This contradicts the ...ClassName conventions used elsewhere
Jira Ticket Link / Motivation
WEB-4205 - Flyout component for Meganav based on the design requirements
Summary of changes
This pull request introduces a new navigation menu component using the
@radix-ui/react-navigation-menupackage and updates related files to integrate and test this new component.New Navigation Menu Component:
package.json: Added@radix-ui/react-navigation-menudependency to the project.src/core/Flyout.tsx: Created a newNavigationMenuDemocomponent using@radix-ui/react-navigation-menuto provide a flexible and styled navigation menu.Storybook Integration:
src/core/Flyout/Flyout.stories.tsx: Added Storybook stories for the newNavigationMenuDemocomponent to demonstrate its usage and styling options.Tailwind Config Update:
Added a new animation
scaleInandscaleOutto be used in the flyout component upon hovering the menu link.Snapshot Testing:
src/core/Flyout/__snapshots__/Flyout.stories.tsx.snap: Updated Jest snapshot tests to include the new navigation menu component, ensuring it renders correctly.How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Dependencies
@radix-ui/react-navigation-menupackage for enhanced navigation capabilities.Storybook