BA-1831: Action overlay #141
Conversation
|
WalkthroughThe changes in this pull request involve the introduction and integration of a new component called Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
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: 8
🧹 Outside diff range and nitpick comments (8)
packages/components/modules/__shared__/ActionsOverlay/styled.tsx (1)
21-27: Consider enhancing mobile responsivenessThe grid layout could be more adaptive for different screen sizes and content lengths:
export const IconButtonContentContainer = styled(Box)(({ theme }) => ({ alignItems: 'center', display: 'grid', - gridTemplateColumns: 'minmax(max-content, 24px) 1fr', + gridTemplateColumns: 'minmax(24px, max-content) minmax(0, 1fr)', gap: theme.spacing(1), alignSelf: 'center', + [theme.breakpoints.down('sm')]: { + gap: theme.spacing(0.5), + }, }))This change:
- Prevents icon column from growing too large
- Ensures text content can shrink if needed
- Adjusts spacing on mobile devices
packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx (1)
22-24: Consider removing or documenting the disabled share actionThe share action is disabled and has an empty click handler. If this is intentional, please add a comment explaining why it's kept. Otherwise, consider removing it to reduce technical debt.
- { - disabled: true, - icon: <LinkIcon />, - label: 'Share Comment', - onClick: () => {}, - hasPermission: true, - closeOnClick: true, - },packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (1)
81-131: Consider adding accessibility improvementsWhile the card implementation is solid, consider enhancing accessibility:
- Add aria-labels for interactive elements
- Include appropriate ARIA roles
- Consider keyboard navigation support
Example improvements:
<StyledChatCard key={`room-${room.id}`} onClick={handleCardClick} isCardSelected={isCardSelected} showPointer={!!handleClick} + role="listitem" + aria-label={`Chat room ${roomData.title}`} + tabIndex={0} >packages/components/CHANGELOG.md (1)
3-8: Enhance the changelog entry with more details.While the entry captures the core change, consider expanding it to better reflect the full scope of changes:
## 0.0.21 ### Patch Changes -Renamed CommentOptions component to ActionsOverlay, and made it reusable. Applied ActionsOverlay on CommentItem and ChatRoom components +- Renamed CommentOptions component to ActionsOverlay for improved reusability +- Enhanced ActionsOverlay with hover-triggered behavior and mobile support +- Integrated ActionsOverlay into CommentItem and ChatRoom components +- Added Storybook documentation for ActionsOverlay componentpackages/components/modules/comments/CommentItem/index.tsx (2)
49-49: Consider keeping plural form for consistency.The state variable
isReplyExpandedmight be clearer asisRepliesExpandedsince it controls the visibility of multiple replies.- const [isReplyExpanded, setIsReplyExpanded] = useState(false) + const [isRepliesExpanded, setIsRepliesExpanded] = useState(false)
137-142: Consider making avatar dimensions configurable.The hardcoded avatar dimensions might affect component reusability across different contexts. Consider making these configurable through props.
+ interface CommentItemProps { + avatarSize?: number; + // ... other props + } const CommentItem: FC<CommentItemProps> = ({ + avatarSize = 40, // ... other props }) => { // ... <AvatarWithPlaceholder - width={40} - height={40} + width={avatarSize} + height={avatarSize} alt={comment.user?.fullName ?? `Comment's user avatar`} src={comment.user?.avatar?.url} />packages/components/modules/__shared__/ActionsOverlay/index.tsx (2)
48-50: Update comments to reflect the current component and variablesThe comments at lines 48-50 reference
comment options's drawerandisLongPressingComment, which seem outdated and may cause confusion. Update the comments to accurately describe theActionsOverlaycomponent and its state variables.Apply this diff to update the comments:
- // This is a workaround to prevent the comment options's drawer from closing when the user clicks on the drawer's content. - // Ideally, we would call setLongPressHandler({ isLongPressingComment: false }) on `onFinished` instead of `onCancel`. - // But, on mobile google chrome devices, the long press click is being wrongly propagated to the backdrop and closing the comment options's drawer right after it opens. + // This is a workaround to prevent the action overlay from closing when the user clicks on the overlay's content. + // Ideally, we would call setLongPressHandler({ isLongPressingItem: false }) on `onFinished` instead of `onCancel`. + // However, on mobile Google Chrome devices, the long press click is incorrectly propagated to the backdrop, closing the action overlay immediately after it opens.
51-53: Simplify type checking of 'className'At lines 51-53, the type checking for
classNamecan be simplified. SinceclassNameis always a string on anHTMLElement, the additional type checking is unnecessary.Apply this diff to simplify the code:
- const className = (e?.target as HTMLElement)?.className || '' - const classNameString = typeof className === 'string' ? className : '' - const isClickOnBackdrop = classNameString.includes('MuiBackdrop') + const className = ((e?.target as HTMLElement)?.className) || '' + const isClickOnBackdrop = className.includes('MuiBackdrop')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between ef0aef0 and 158e6e27e0f401791950bc2e0eae5335879e1c61.
📒 Files selected for processing (13)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/styled.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(1 hunks)packages/components/modules/comments/CommentItem/CommentOptions/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/types.ts(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(4 hunks)packages/components/modules/comments/CommentItem/types.ts(0 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx(2 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/types.ts(0 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(3 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/comments/CommentItem/CommentOptions/index.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/types.ts
- packages/components/modules/comments/CommentItem/types.ts
- packages/components/modules/comments/CommentItem/useCommentOptions/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🔇 Additional comments (12)
packages/components/modules/__shared__/ActionsOverlay/styled.tsx (2)
1-4: LGTM! Imports are clean and well-organized.
1-27: Verify hover timing requirements
The PR requirements specify that the overlay should:
- Appear within 1 second of hovering
- Disappear immediately when the mouse leaves
Please ensure that the parent component implementing this styled component handles these timing requirements correctly.
packages/components/modules/__shared__/ActionsOverlay/types.ts (2)
16-19: LGTM! Good mobile interaction support
The LongPressHandler type properly handles mobile interactions, aligning with the requirement to support mobile behavior.
33-36: LGTM! Clean container props interface
The interface is appropriately focused on positioning concerns.
packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx (3)
32-32: LGTM!
The addition of closeOnClick property is consistent with the overlay behavior requirements.
40-40: LGTM!
The addition of closeOnClick property and simplified click handler improve code clarity.
3-3: Verify the component's usage context
The hook returns comment-specific actions (share, pin, edit) rather than the chat room actions (archive, mark as unread, delete) specified in the PR objectives. Please clarify if this component is being repurposed for chat rooms or if this is the correct implementation.
Also applies to: 7-7
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (2)
Line range hint 1-31: LGTM! Hook and imports are properly implemented.
The addition of useRef and necessary imports for the overlay functionality is well-structured.
Line range hint 1-132: Verify mobile responsiveness implementation
The PR objectives specify that the component must be consistent across all devices and reuse a component that supports mobile behavior. However, there's no explicit mobile-specific handling visible in the code.
✅ Verification successful
Mobile responsiveness is properly implemented
The component is built with proper mobile responsiveness through multiple layers:
- The ChatRoomItem itself has mobile-specific styling:
[theme.breakpoints.down('sm')]: {
maxWidth: '600px',
padding: theme.spacing(1.5),
}- The ActionsOverlay component (which wraps ChatRoomItem) has built-in mobile support:
- Uses SwipeableDrawer for mobile interaction
- Handles mobile-specific long-press events
- Uses responsive MUI components (Box, Grid) with proper styling
- The component inherits from the design system's breakpoint configurations, as evidenced by the tailwind config using standardized breakpoints.
The implementation satisfies both requirements:
- Reuses components that support mobile behavior (ActionsOverlay with SwipeableDrawer)
- Maintains consistency across devices through responsive design patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mobile-specific implementations or responsive design patterns
rg -l 'useMediaQuery|@media|theme.breakpoints' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 103
Script:
#!/bin/bash
# Let's check the styled components and mobile-related implementations in both ChatRoomItem and ActionsOverlay
rg -A 5 '(styled|@media|breakpoints|useMediaQuery|mobile)' packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.ts
# Also check ActionsOverlay's implementation
rg -A 5 '(styled|@media|breakpoints|useMediaQuery|mobile)' packages/components/modules/__shared__/ActionsOverlay/styled.ts
# Check if there are any mobile-specific props or interfaces
ast-grep --pattern 'interface $_ {
$$$
mobile?: $_
$$$
}'
Length of output: 543
Script:
#!/bin/bash
# Let's try to find the styled files with a more flexible search
fd -e ts -e tsx styled
# Search for any mobile-related implementations in the entire components directory
rg -A 5 '(useMediaQuery|@media|breakpoints|mobile)' packages/components/
# Check if ActionsOverlay has any responsive design implementations
rg -A 5 'Box|Grid|Container' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 29762
packages/components/modules/comments/CommentItem/index.tsx (3)
12-12: LGTM! Import changes align with component reuse strategy.
The introduction of ActionsOverlay and removal of CommentOptions aligns with the PR objectives of reusing existing components.
128-165: LGTM! Overlay implementation meets requirements.
The ActionsOverlay implementation successfully integrates the required functionality while maintaining a clean component structure.
128-133: Verify hover behavior matches requirements.
The implementation looks good, but we should verify that the overlay appears within one second of hovering and disappears immediately when the mouse moves away.
| export const ActionOverlayContainer = styled(Box)<ActionOverlayContainerProps>( | ||
| ({ theme, offsetTop = 0, offsetRight = 0 }) => ({ | ||
| backgroundColor: theme.palette.background.default, | ||
| border: `1px solid ${theme.palette.divider}`, | ||
| borderRadius: theme.spacing(1), | ||
| display: 'flex', | ||
| gap: theme.spacing(1), | ||
| padding: theme.spacing(0.75, 1), | ||
| position: 'absolute', | ||
| right: 12 - offsetRight, | ||
| top: -12 - offsetTop, | ||
| zIndex: 1, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improvements for responsiveness and accessibility
The ActionOverlayContainer implementation has several areas for improvement:
- Replace fixed pixel values with theme spacing for consistency:
- right: 12 - offsetRight,
- top: -12 - offsetTop,
+ right: theme.spacing(1.5) - offsetRight,
+ top: -theme.spacing(1.5) - offsetTop,- Consider adding hover transition for smooth appearance as per requirements:
position: 'absolute',
right: theme.spacing(1.5) - offsetRight,
top: -theme.spacing(1.5) - offsetTop,
zIndex: 1,
+ transition: theme.transitions.create(['opacity', 'visibility'], {
+ duration: theme.transitions.duration.shorter,
+ }),- The z-index value of 1 might be too low. Consider using theme.zIndex for consistent layering:
- zIndex: 1,
+ zIndex: theme.zIndex.tooltip,- Add ARIA attributes for accessibility:
position: 'absolute',
+ role: 'menu',
+ 'aria-label': 'Action options',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const ActionOverlayContainer = styled(Box)<ActionOverlayContainerProps>( | |
| ({ theme, offsetTop = 0, offsetRight = 0 }) => ({ | |
| backgroundColor: theme.palette.background.default, | |
| border: `1px solid ${theme.palette.divider}`, | |
| borderRadius: theme.spacing(1), | |
| display: 'flex', | |
| gap: theme.spacing(1), | |
| padding: theme.spacing(0.75, 1), | |
| position: 'absolute', | |
| right: 12 - offsetRight, | |
| top: -12 - offsetTop, | |
| zIndex: 1, | |
| }), | |
| ) | |
| export const ActionOverlayContainer = styled(Box)<ActionOverlayContainerProps>( | |
| ({ theme, offsetTop = 0, offsetRight = 0 }) => ({ | |
| backgroundColor: theme.palette.background.default, | |
| border: `1px solid ${theme.palette.divider}`, | |
| borderRadius: theme.spacing(1), | |
| display: 'flex', | |
| gap: theme.spacing(1), | |
| padding: theme.spacing(0.75, 1), | |
| position: 'absolute', | |
| right: theme.spacing(1.5) - offsetRight, | |
| top: -theme.spacing(1.5) - offsetTop, | |
| role: 'menu', | |
| 'aria-label': 'Action options', | |
| transition: theme.transitions.create(['opacity', 'visibility'], { | |
| duration: theme.transitions.duration.shorter, | |
| }), | |
| zIndex: theme.zIndex.tooltip, | |
| }), | |
| ) |
There was a problem hiding this comment.
@pedrotpo I think that's worth doing as well
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| export interface ActionOverlayProps extends BoxProps { | ||
| ContainerProps?: BoxProps | ||
| enableDelete?: boolean | ||
| isDeletingItem?: boolean | ||
| handleDeleteItem?: () => void | ||
| actions: OverlayAction[] | ||
| SwipeableDrawer?: FC<SwipeableDrawerProps> | ||
| SwipeableDrawerProps?: Partial<SwipeableDrawerProps> | ||
| offsetTop?: number | ||
| offsetRight?: number | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing properties to meet overlay requirements
The interface needs additional properties to fully meet the requirements:
- Animation timing control (should appear within 1 second)
- Mouse leave handling (should disappear immediately)
- Z-index control for proper overlay positioning
export interface ActionOverlayProps extends BoxProps {
ContainerProps?: BoxProps
enableDelete?: boolean
isDeletingItem?: boolean
handleDeleteItem?: () => void
- actions: OverlayAction[]
+ actions: NonEmptyArray<OverlayAction> // ensure array is not empty
SwipeableDrawer?: FC<SwipeableDrawerProps>
SwipeableDrawerProps?: Partial<SwipeableDrawerProps>
offsetTop?: number
offsetRight?: number
+ zIndex?: number
+ appearanceDelay?: number // in milliseconds, default should be < 1000
+ onMouseLeave?: () => void
}Also, consider adding a type guard:
type NonEmptyArray<T> = [T, ...T[]];| export type OverlayAction = { | ||
| label: string | ||
| icon: JSX.Element | ||
| onClick: () => void | ||
| disabled: boolean | ||
| hasPermission?: boolean | null | ||
| closeOnClick?: boolean | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type definition for better accessibility and testing
Consider the following improvements to the OverlayAction type:
- Make
disabledoptional with a default value - Add accessibility properties
- Add testing identifier
export type OverlayAction = {
label: string
icon: JSX.Element
onClick: () => void
- disabled: boolean
+ disabled?: boolean
hasPermission?: boolean | null
closeOnClick?: boolean
+ ariaLabel?: string
+ dataTestId?: string
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type OverlayAction = { | |
| label: string | |
| icon: JSX.Element | |
| onClick: () => void | |
| disabled: boolean | |
| hasPermission?: boolean | null | |
| closeOnClick?: boolean | |
| } | |
| export type OverlayAction = { | |
| label: string | |
| icon: JSX.Element | |
| onClick: () => void | |
| disabled?: boolean | |
| hasPermission?: boolean | null | |
| closeOnClick?: boolean | |
| ariaLabel?: string | |
| dataTestId?: string | |
| } |
| import { FC } from 'react' | ||
|
|
||
| import type { SwipeableDrawerProps } from '@baseapp-frontend/design-system' | ||
|
|
||
| import { BoxProps } from '@mui/material' | ||
|
|
||
| export type OverlayAction = { | ||
| label: string | ||
| icon: JSX.Element | ||
| onClick: () => void | ||
| disabled: boolean | ||
| hasPermission?: boolean | null | ||
| closeOnClick?: boolean | ||
| } | ||
|
|
||
| export type LongPressHandler = { | ||
| isLongPressingItem: boolean | ||
| shouldOpenItemOptions: boolean | ||
| } | ||
|
|
||
| export interface ActionOverlayProps extends BoxProps { | ||
| ContainerProps?: BoxProps | ||
| enableDelete?: boolean | ||
| isDeletingItem?: boolean | ||
| handleDeleteItem?: () => void | ||
| actions: OverlayAction[] | ||
| SwipeableDrawer?: FC<SwipeableDrawerProps> | ||
| SwipeableDrawerProps?: Partial<SwipeableDrawerProps> | ||
| offsetTop?: number | ||
| offsetRight?: number | ||
| } | ||
|
|
||
| export interface ActionOverlayContainerProps extends BoxProps { | ||
| offsetTop?: number | ||
| offsetRight?: number | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing Storybook documentation for ActionsOverlay component
The component lacks Storybook documentation as evidenced by:
- No
.stories.tsxor.stories.tsfiles found in the components directory - No MDX documentation files referencing the component
- No documentation files present in the component's directory
Please add Storybook documentation for the ActionsOverlay component to meet the PR requirements. The documentation should showcase:
- Basic usage
- Different action configurations
- Various prop combinations
- Edge cases (e.g., disabled states, permission states)
🔗 Analysis chain
Verify Storybook documentation existence
The PR requirements mention that the component must be documented in Storybook. Let's verify this requirement is met.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Storybook documentation file
fd -e stories.tsx -e stories.ts "ActionsOverlay"
Length of output: 48
Script:
#!/bin/bash
# Let's try a broader search for any stories files in the components directory
fd -e stories.tsx -e stories.ts . packages/components/
# Also check if the component is referenced in any MDX files (another common Storybook format)
fd -e mdx . packages/components/ --exec grep -l "ActionsOverlay"
# Finally, let's check the component directory for any documentation
ls -la packages/components/modules/__shared__/ActionsOverlay/
Length of output: 474
There was a problem hiding this comment.
@pt-tsl it would be cool if we could do the storybook for this one, especially now that it is a reusable component
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <ActionsOverlay | ||
| title="Chat" | ||
| offsetTop={-12} | ||
| actions={[ | ||
| { | ||
| disabled: true, | ||
| icon: <LinkIcon />, | ||
| label: 'Share Comment', | ||
| onClick: () => console.log('Share'), | ||
| hasPermission: true, | ||
| }, | ||
| { | ||
| disabled: false, | ||
| icon: <PenEditIcon />, | ||
| label: 'Edit Comment', | ||
| onClick: () => console.log('Edit'), | ||
| hasPermission: true, | ||
| }, | ||
| ]} | ||
| enableDelete | ||
| handleDeleteItem={() => console.log('Delete')} | ||
| isDeletingItem={false} | ||
| ref={chatCardRef} | ||
| > |
There was a problem hiding this comment.
Implementation doesn't match requirements
The current implementation has several discrepancies with the PR objectives:
- The actions implemented (share/edit) don't match the required actions (archive/mark as unread/delete)
- Console.log statements in onClick handlers suggest incomplete implementation
- No clear indication of hover behavior implementation
Consider this implementation instead:
<ActionsOverlay
title="Chat"
offsetTop={-12}
actions={[
{
- disabled: true,
- icon: <LinkIcon />,
- label: 'Share Comment',
- onClick: () => console.log('Share'),
+ disabled: false,
+ icon: <ArchiveIcon />,
+ label: 'Archive Chat',
+ onClick: onArchive,
hasPermission: true,
},
{
disabled: false,
- icon: <PenEditIcon />,
- label: 'Edit Comment',
- onClick: () => console.log('Edit'),
+ icon: <UnreadIcon />,
+ label: 'Mark as Unread',
+ onClick: onMarkUnread,
hasPermission: true,
},
]}
enableDelete
- handleDeleteItem={() => console.log('Delete')}
+ handleDeleteItem={onDelete}
isDeletingItem={false}
ref={chatCardRef}
>Committable suggestion skipped: line range outside the PR's diff.
| }) | ||
|
|
||
| const longPressHandlers = useLongPress<HTMLDivElement>( | ||
| (e: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the 'any' type for event parameters
At line 42, the event parameter e is typed as any. Using any reduces type safety and may mask potential issues. Specify a more precise event type to enhance type checking and maintainability.
Apply this diff to specify the correct event type:
- (e: any) => {
+ (e: React.MouseEvent<HTMLDivElement> | React.TouchEvent<HTMLDivElement>) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (e: any) => { | |
| (e: React.MouseEvent<HTMLDivElement> | React.TouchEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| setIsDeleteDialogOpen(true) | ||
| } | ||
|
|
||
| const deviceHasHover = window.matchMedia('(hover: hover)').matches |
There was a problem hiding this comment.
Ensure safe usage of 'window' for server-side rendering compatibility
At line 70, using window.matchMedia without checking if window is defined may cause errors during server-side rendering (SSR), where window is undefined. To prevent this issue, add a check to ensure window exists before using it.
Apply this diff to fix the issue:
- const deviceHasHover = window.matchMedia('(hover: hover)').matches
+ const deviceHasHover = typeof window !== 'undefined' && window.matchMedia('(hover: hover)').matches📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const deviceHasHover = window.matchMedia('(hover: hover)').matches | |
| const deviceHasHover = typeof window !== 'undefined' && window.matchMedia('(hover: hover)').matches |
There was a problem hiding this comment.
@pedrotpo that's a good suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
pt-tsl
force-pushed
the
BA-1831-action-overlay
branch
from
158e6e2 to
33e55c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/components/modules/comments/CommentItem/index.tsx (3)
49-49: Consider aligning state variable name with query parameter.The state variable
isReplyExpandedis used with a query parameter namedisRepliesExpanded. This inconsistency could lead to confusion. Consider using consistent naming.- const [isReplyExpanded, setIsReplyExpanded] = useState(false) + const [isRepliesExpanded, setIsRepliesExpanded] = useState(false)
Line range hint
63-74: Enhance error handling with user feedback.While error logging is present, users won't be notified when reply expansion fails. Consider adding user-facing error notifications.
onComplete: (error) => { if (error) { console.error(error) + // Add user notification here + // Example: showErrorNotification('Failed to load replies') } else { setIsReplyExpanded(true) } },
128-165: Add accessibility attributes to the overlay.The overlay should include proper ARIA attributes for better accessibility.
<ActionsOverlay actions={defaultCommentOptions} enableDelete={enableDelete && !!comment?.canDelete} handleDeleteItem={handleDeleteComment} isDeletingItem={isDeletingComment} title="Comment" + aria-label="Comment actions" + role="menu" ref={commentItemRef} >packages/components/modules/__shared__/ActionsOverlay/index.tsx (3)
48-50: Typographical corrections in commentsThe comments in lines 48-50 contain typographical errors:
- "comment options's drawer" should be "comment options drawer" or "comment options' drawer".
- "mobile google chrome devices" should be "mobile Google Chrome devices".
51-53: Simplify className type checkingIn lines 51-53, the type checking for
classNamecan be simplified. SinceclassNameis expected to be a string, andHTMLElement.classNamereturns a string, the additional type check may be unnecessary.Apply this diff to simplify the code:
- const className = (e?.target as HTMLElement)?.className || '' - const classNameString = typeof className === 'string' ? className : '' - const isClickOnBackdrop = classNameString.includes('MuiBackdrop') + const className = (e?.target as HTMLElement)?.className || '' + const isClickOnBackdrop = className.includes('MuiBackdrop')
210-212: Ensure prop spread order to prevent unintended overridesIn lines 210-212, props from
longPressHandlers()andContainerPropsare spread into theBoxcomponent. Be cautious of the spread order to ensure that props are not unintentionally overridden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 158e6e27e0f401791950bc2e0eae5335879e1c61 and 33e55c579361601f7688f335e8cd0854b2dc0c21.
📒 Files selected for processing (11)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(1 hunks)packages/components/modules/comments/CommentItem/CommentOptions/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/types.ts(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(4 hunks)packages/components/modules/comments/CommentItem/types.ts(0 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx(2 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/types.ts(0 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(3 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/comments/CommentItem/CommentOptions/index.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/types.ts
- packages/components/modules/comments/CommentItem/types.ts
- packages/components/modules/comments/CommentItem/useCommentOptions/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/CHANGELOG.md
- packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx
- packages/components/package.json
🔇 Additional comments (9)
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (4)
31-32: LGTM!
The ref implementation is correct and necessary for the ActionsOverlay component.
57-80: Implementation doesn't match requirements
The current implementation still has discrepancies with the PR objectives:
- Actions don't match requirements (share/edit vs archive/mark as unread/delete)
- Console.log statements need to be replaced with proper stubs
- Hover behavior isn't implemented
Additionally, consider adding hover behavior props:
<ActionsOverlay
title="Chat"
offsetTop={-12}
+ showOnHover={true}
+ hoverDelay={1000}
actions={[...]}
>81-132: LGTM!
The UI structure maintains all necessary elements while properly integrating the overlay component. The layout is well-organized and preserves the existing functionality.
9-9: Verify mobile behavior support
Please confirm that the imported ActionsOverlay from __shared__ is the same component mentioned in the requirements that supports mobile behavior.
✅ Verification successful
Verified: ActionsOverlay component has proper mobile support
The imported ActionsOverlay component from __shared__ is indeed the correct component with mobile behavior support, as evidenced by:
- It uses device detection to provide different UX for mobile vs desktop:
deviceHasHover = window.matchMedia('(hover: hover').matches - For mobile devices (non-hover), it implements a long-press interaction with
SwipeableDrawercomponent - For desktop (hover devices), it shows a hover-based overlay with action buttons
- Mobile-specific features include:
- Long press gesture detection with 400ms threshold
- Touch-friendly bottom drawer interface
- Mobile-optimized layout with larger touch targets and clear labels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ActionsOverlay is the reused component with mobile support
# Look for mobile-related props or implementations in ActionsOverlay
ast-grep --pattern 'const ActionsOverlay = $$$' '../../../__shared__/ActionsOverlay/index.tsx'
# Check for mobile-related styles
rg -A 5 'mobile|touch|responsive' '../../../__shared__/ActionsOverlay/styled.tsx'
Length of output: 354
Script:
#!/bin/bash
# Let's try searching from the repository root with fd and rg
# Find ActionsOverlay files
fd -t f "ActionsOverlay"
# Look for mobile-related implementations in ActionsOverlay files
rg -A 5 'mobile|touch|responsive' $(fd -t f "ActionsOverlay")
# Look for ActionsOverlay component definition
ast-grep --pattern 'const ActionsOverlay = $$$'
Length of output: 49162
packages/components/modules/comments/CommentItem/index.tsx (2)
12-12: LGTM! Import changes align with the new overlay functionality.
Also applies to: 16-16
60-61:
Add error handling for delete mutation.
The delete operation should handle potential errors to provide feedback to users and maintain a consistent state.
- const [deleteComment, isDeletingComment] = useCommentDeleteMutation()
+ const [deleteComment, isDeletingComment] = useCommentDeleteMutation({
+ onError: (error) => {
+ console.error('Failed to delete comment:', error);
+ // Add user notification here
+ }
+ })packages/components/modules/__shared__/ActionsOverlay/index.tsx (3)
42-42: Avoid using the any type for event parameters
At line 42, the event parameter e is typed as any. Using any reduces type safety and may mask potential issues. Specify a more precise event type to enhance type checking and maintainability.
70-70: Ensure safe usage of window for server-side rendering compatibility
At line 70, using window.matchMedia without checking if window is defined may cause errors during server-side rendering (SSR), where window is undefined. To prevent this issue, add a check to ensure window exists before using it.
205-219: Overall implementation looks good
The ActionsOverlay component is well-structured and effectively handles both hover and long-press interactions to display action overlays. The logic is clear, and the use of hooks and props promotes reusability and maintainability.
| @@ -1,11 +1,12 @@ | |||
| import { FC, SyntheticEvent } from 'react' | |||
| import { FC, SyntheticEvent, useRef } from 'react' | |||
There was a problem hiding this comment.
Update imports to match required actions
The current imports (LinkIcon, PenEditIcon) don't align with the required actions (archive, mark as unread, delete). Consider importing the appropriate icons instead.
-import { AvatarWithPlaceholder, LinkIcon, PenEditIcon } from '@baseapp-frontend/design-system'
+import { AvatarWithPlaceholder, ArchiveIcon, UnreadIcon } from '@baseapp-frontend/design-system'Also applies to: 3-3, 9-9
| aria-label="actions overlay" | ||
| {...SwipeableDrawerProps} | ||
| > | ||
| <div className="grid grid-cols-[1fr] justify-start gap-2"> |
There was a problem hiding this comment.
let's covert that to a Box styled, please
| setIsDeleteDialogOpen(true) | ||
| } | ||
|
|
||
| const deviceHasHover = window.matchMedia('(hover: hover)').matches |
There was a problem hiding this comment.
@pedrotpo that's a good suggestion
| export const ActionOverlayContainer = styled(Box)<ActionOverlayContainerProps>( | ||
| ({ theme, offsetTop = 0, offsetRight = 0 }) => ({ | ||
| backgroundColor: theme.palette.background.default, | ||
| border: `1px solid ${theme.palette.divider}`, | ||
| borderRadius: theme.spacing(1), | ||
| display: 'flex', | ||
| gap: theme.spacing(1), | ||
| padding: theme.spacing(0.75, 1), | ||
| position: 'absolute', | ||
| right: 12 - offsetRight, | ||
| top: -12 - offsetTop, | ||
| zIndex: 1, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
@pedrotpo I think that's worth doing as well
| @@ -143,57 +117,54 @@ const CommentItem: FC<CommentItemProps> = ({ | |||
| return null | |||
| } | |||
|
|
|||
| const { options = defaultCommentOptions, ...restOfCommentOptionsProps } = | |||
| CommentOptionsProps ?? {} | |||
There was a problem hiding this comment.
it would be nice if we could keep this and add a ActionsOverlayProps instead
pt-tsl
force-pushed
the
BA-1831-action-overlay
branch
from
33e55c5 to
f2d8eb6
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsx (2)
1-6: Consider importing action icons for the overlay.Based on the PR objectives, the overlay should demonstrate archive, mark as unread, and delete actions. Consider importing the corresponding icons from your icon library to make the example more representative of the actual usage.
import React, { RefAttributes } from 'react' import { Button } from '@mui/material' +import { ArchiveIcon, UnreadIcon, DeleteIcon } from '@your-icon-library' import ActionsOverlay from '../..' import { ActionOverlayProps } from '../../types'
8-10: Add JSDoc documentation for the component.Since this is a Storybook example component, adding documentation will help other developers understand its purpose and usage better.
+/** + * A Storybook example component demonstrating the ActionsOverlay functionality. + * Shows how to integrate ActionsOverlay with a button component. + * @example + * <ActionsOverlayOnButton + * actions={[ + * { label: 'Archive', icon: <ArchiveIcon />, onClick: () => {} }, + * { label: 'Mark as Unread', icon: <UnreadIcon />, onClick: () => {} }, + * { label: 'Delete', icon: <DeleteIcon />, onClick: () => {} } + * ]} + * /> + */ const ActionsOverlayOnButton = ( props: Omit<ActionOverlayProps, 'ref'> & RefAttributes<HTMLDivElement>, ) => {packages/design-system/components/icons/ArchiveIcon/index.tsx (2)
6-6: Consider making the icon size configurableWhile the current implementation works, consider making the icon size configurable through props to enhance reusability across different contexts. The hardcoded size of 18 might not suit all use cases.
-const ArchiveIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( +const ArchiveIcon: FC<SvgIconProps & { size?: number }> = ({ sx, size = 18, ...props }) => (Then use the size prop:
- <SvgIcon sx={{ fontSize: 18, color: 'action.active', ...sx }} {...props}> + <SvgIcon sx={{ fontSize: size, color: 'action.active', ...sx }} {...props}>
7-16: Add accessibility attributes to the SVGTo improve accessibility, consider adding ARIA attributes and a title element to the SVG.
- <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 18 18" fill="none"> + <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 18 18" fill="none" + role="img" aria-label="Archive"> + <title>Archive</title> <pathpackages/design-system/components/icons/UnreadIcon/index.tsx (2)
5-6: Consider using theme tokens for fontSize.The implementation looks good, but the hardcoded fontSize of 18 could be moved to the theme system for better maintainability and consistency.
Consider updating the sx prop to use a theme token:
- <SvgIcon sx={{ fontSize: 18, color: 'action.active', ...sx }} {...props}> + <SvgIcon sx={{ fontSize: theme => theme.typography.pxToRem(18), color: 'action.active', ...sx }} {...props}>
7-44: Add accessibility attributes and documentation.While the SVG implementation is correct, consider the following improvements:
- Add ARIA attributes for better accessibility
- Add JSDoc comments explaining the visual representation
Consider adding these improvements:
+ /** + * UnreadIcon - Represents a slashed eye icon indicating the unread state + * Visual: An eye symbol with a diagonal line through it + */ const UnreadIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( <SvgIcon sx={{ fontSize: 18, color: 'action.active', ...sx }} {...props}> - <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 18 18" fill="none"> + <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 18 18" fill="none" + role="img" + aria-label="Mark as unread">packages/components/CHANGELOG.md (1)
7-7: Consider enhancing the changelog entry with more details.While the current entry captures the core changes, it would be helpful to add:
- The purpose of the ActionsOverlay (e.g., "to provide hover-activated action buttons")
- The specific actions it supports (archive, mark as unread, delete)
- Any notable UI/UX aspects (e.g., "appears on hover within 1s")
Example enhancement:
- Removed CommentOptions from CommentItem component and refactored into ActionsOverlay. Applied ActionsOverlay to CommentItem and ChatRoomItem components. + Removed CommentOptions from CommentItem component and refactored into ActionsOverlay - a hover-activated component providing archive, mark as unread, and delete actions. Applied ActionsOverlay to both CommentItem and ChatRoomItem components with a 1-second hover trigger.packages/design-system/CHANGELOG.md (1)
3-8: Enhance the changelog entry with more context.While the changelog entry correctly documents the addition of new icons, it could be more descriptive to help other developers understand their purpose and usage.
Consider expanding the entry like this:
## 0.0.22 ### Patch Changes -Added Archive and Unread icons +Added Archive and Unread icons for use in chat room action overlayspackages/components/modules/comments/CommentItem/index.tsx (2)
34-34: Add JSDoc comments for the new props.Consider adding JSDoc comments to document the purpose and usage of
ActionOverlayPropsandenableDeleteprops for better maintainability.+/** Props to customize the actions overlay behavior and appearance */ ActionOverlayProps, +/** Whether to enable the delete functionality. Defaults to true */ enableDelete = true,
132-170: Enhance accessibility attributes.Consider adding the following accessibility improvements:
- ARIA labels for interactive elements
- Role attributes for semantic structure
- Keyboard navigation support
<ActionsOverlay actions={actions} enableDelete={enableDelete && !!comment?.canDelete} handleDeleteItem={handleDeleteComment} isDeletingItem={isDeletingComment} title="Comment" + aria-label={`Comment by ${comment.user?.fullName}`} {...restOfActionOverlayProps} ref={commentItemRef} > - <CommentContainer> + <CommentContainer role="article"> <AvatarWithPlaceholder width={40} height={40} alt={comment.user?.fullName ?? `Comment's user avatar`} src={comment.user?.avatar?.url} /> - <div className="grid gap-3"> + <div className="grid gap-3" role="region" aria-label="Comment content">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 33e55c579361601f7688f335e8cd0854b2dc0c21 and f2d8eb6.
📒 Files selected for processing (24)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlay.mdx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/styled.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(1 hunks)packages/components/modules/comments/CommentItem/CommentOptions/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentOptions/types.ts(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(5 hunks)packages/components/modules/comments/CommentItem/types.ts(2 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx(2 hunks)packages/components/modules/comments/CommentItem/useCommentOptions/types.ts(0 hunks)packages/components/modules/comments/Comments/__tests__/Comments.cy.tsx(2 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(3 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/icons/ArchiveIcon/index.tsx(1 hunks)packages/design-system/components/icons/UnreadIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (4)
- packages/components/modules/comments/CommentItem/CommentOptions/index.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/styled.tsx
- packages/components/modules/comments/CommentItem/CommentOptions/types.ts
- packages/components/modules/comments/CommentItem/useCommentOptions/types.ts
✅ Files skipped from review due to trivial changes (3)
- packages/design-system/package.json
- packages/wagtail/CHANGELOG.md
- packages/wagtail/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/components/modules/shared/ActionsOverlay/index.tsx
- packages/components/modules/shared/ActionsOverlay/styled.tsx
- packages/components/modules/shared/ActionsOverlay/types.ts
- packages/components/modules/comments/CommentItem/types.ts
- packages/components/modules/comments/CommentItem/useCommentOptions/index.tsx
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/package.json
🧰 Additional context used
🪛 LanguageTool
packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlay.mdx
[misspelling] ~19-~19: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...a delete action, the component provides a enabler, loading and click handler prop...
(EN_A_VS_AN)
🔇 Additional comments (13)
packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsx (1)
1-19: Verify mobile behavior compatibility.
The PR objectives mention reusing a component that supports mobile behavior. Please verify that this example demonstrates proper mobile interaction patterns.
✅ Verification successful
Let me gather more information about the implementation details to verify if the mobile behavior is properly handled.
Let me check one more aspect to verify the long press implementation.
Based on the gathered information, I can now provide the final response:
Mobile behavior is properly implemented in the base component
The example correctly demonstrates mobile interaction patterns as it uses the base ActionsOverlay component which includes:
- Mobile detection via
matchMedia('(hover: hover') - SwipeableDrawer implementation for mobile view
- Long press handling with proper mobile-specific fixes (e.g., Chrome touch event propagation)
- Documented mobile behavior in the component's MDX file
The story serves its purpose by inheriting all mobile functionality from the base component without requiring additional mobile-specific code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mobile-related props or documentation in the ActionsOverlay component
rg -A 5 'mobile|touch|responsive' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 2223
Script:
#!/bin/bash
# Check for long press and drawer related implementations
ast-grep --pattern 'const $_ = useLongPress' packages/components/modules/__shared__/ActionsOverlay/index.tsx
# Check for drawer component usage
rg -A 5 'Drawer|SwipeableDrawer' packages/components/modules/__shared__/ActionsOverlay/
# Check for any mobile detection logic
rg 'isMobile|useMediaQuery|matchMedia' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 5999
Script:
#!/bin/bash
# Check for long press hook implementation and mobile-specific logic
ast-grep --pattern 'const $_ = {
$$$
isLongPressingItem: $_,
$$$
}' packages/components/modules/__shared__/ActionsOverlay/index.tsx
# Check for any touch event handlers
rg 'onTouchStart|onTouchEnd|onTouchMove' packages/components/modules/__shared__/ActionsOverlay/
Length of output: 237
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx (1)
8-11: Verify the correct component for the story
There's an inconsistency in the metadata:
- The Meta type refers to
ActionsOverlay - The component property points to
ActionsOverlayOnButton
This might lead to confusion. Please clarify if this is intentional or needs to be corrected.
✅ Verification successful
The component usage in the story is correct
The implementation shows that ActionsOverlayOnButton is a valid wrapper component that demonstrates ActionsOverlay usage with a button. This is a common pattern in Storybook where a wrapper component is used to showcase the main component in a practical context:
ActionsOverlayOnButtonproperly implements and forwards all props toActionsOverlay- The story metadata correctly uses
ActionsOverlaytype for props while using the demo wrapper as the displayed component - This setup allows developers to see how
ActionsOverlayworks in a real-world scenario with a button
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ActionsOverlayOnButton is a wrapper or test utility
cat packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton.tsx
Length of output: 227
Script:
#!/bin/bash
# Let's first check what files exist in the storybook directory
ls -la packages/components/modules/__shared__/ActionsOverlay/__storybook__/
# Also search for ActionsOverlayOnButton definition in the codebase
rg "ActionsOverlayOnButton" -l
Length of output: 601
Script:
#!/bin/bash
# Let's check the content of both files to understand the relationship
cat packages/components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsx
cat packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx
Length of output: 1603
packages/design-system/components/icons/index.ts (1)
16-18: Verify the reinstated icon components.
The reinstated icons (PenEditIcon, PinIcon, SendMessageIcon, TrashCanIcon) complement the overlay functionality.
Let's verify the usage of these icons in the ActionsOverlay component:
Also applies to: 20-20
✅ Verification successful
Icons are correctly reinstated and actively used in the codebase
The icons are verified to be properly integrated and used across multiple components:
PenEditIcon: Used in comment editing functionality (CommentItem/useCommentOptions)PinIcon: Used in comment pinning features (CommentItem/useCommentOptions, CommentPinnedBadge)SendMessageIcon: Used in SocialInput/SubmitActions and navigation componentsTrashCanIcon: Used in ActionsOverlay component for deletion actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of reinstated icons in ActionsOverlay
# Search for imports and usage in ActionsOverlay
echo "Checking icon usage in ActionsOverlay..."
rg -A 5 "PenEditIcon|PinIcon|SendMessageIcon|TrashCanIcon" --type ts --type tsx
Length of output: 200
Script:
#!/bin/bash
# Let's try a different approach to search for these icons
# First, let's find all TypeScript and JavaScript files that might contain these icons
echo "Searching for icon usage in all TypeScript and JavaScript files..."
rg -l "PenEditIcon|PinIcon|SendMessageIcon|TrashCanIcon" --type ts --type js
# Let's also check if there are any React/TSX files specifically
echo -e "\nChecking for TSX/JSX files containing these icons..."
fd -e tsx -e jsx . | xargs rg "PenEditIcon|PinIcon|SendMessageIcon|TrashCanIcon"
# Let's also verify if these icon files actually exist
echo -e "\nVerifying icon files existence..."
fd -e ts -e tsx "PenEditIcon|PinIcon|SendMessageIcon|TrashCanIcon"
Length of output: 4253
packages/design-system/components/icons/ArchiveIcon/index.tsx (2)
1-5: LGTM! Clean imports and proper TypeScript typing
The component is well-structured with appropriate type definitions using React.FC and Material-UI's SvgIconProps.
1-20: Verify Storybook documentation
The PR objectives require documentation in Storybook for new components. Let's verify this requirement is met.
✅ Verification successful
Icon is properly documented in Storybook
The ArchiveIcon is correctly integrated into the design system's documentation:
- It's exported in the main icons index file
- It follows the documented icon creation guidelines from the README
- It's automatically included in the Storybook's Iconography page through the dynamic IconGallery component that renders all exported icons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Storybook stories for ArchiveIcon
fd -e stories.tsx -e stories.ts -e stories.jsx -e stories.js . | grep -i "ArchiveIcon"
# Check if the component is exported in any index files for Storybook
rg -l "export.*ArchiveIcon" "packages/design-system/**/*.stories.*"
Length of output: 234
Script:
#!/bin/bash
# Let's first check if there are any story files in the design system
fd -e stories.tsx -e stories.ts -e stories.jsx -e stories.js . packages/design-system/
# Let's also check the component's directory structure
ls -la packages/design-system/components/icons/ArchiveIcon/
# Check if there are any story files for icons in general
fd -e stories.tsx -e stories.ts -e stories.jsx -e stories.js . packages/design-system/components/icons/
Length of output: 429
Script:
#!/bin/bash
# Let's check if there's a common story file for all icons
fd "icons?.stories" packages/design-system/
# Check the structure of other icon components to understand the pattern
ls -R packages/design-system/components/icons/
# Look for any documentation patterns in the design system
fd "README|DOCUMENTATION" packages/design-system/
Length of output: 2154
Script:
#!/bin/bash
# Let's check the content of the icons documentation
cat packages/design-system/components/icons/__storybook__/Iconography.mdx
cat packages/design-system/components/icons/README.md
# Check if ArchiveIcon is exported in the icons index
cat packages/design-system/components/icons/index.ts
Length of output: 4381
packages/design-system/components/icons/UnreadIcon/index.tsx (3)
1-3: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary types and components.
48-48: LGTM! Clean export statement.
The default export is appropriate for this component.
1-48: Verify Storybook documentation requirement.
The PR objectives mention that new components must be documented in Storybook. Please ensure that a story is created for this component.
✅ Verification successful
Storybook documentation is already implemented correctly
The UnreadIcon is automatically included in the Iconography documentation through the central Iconography.mdx file, which dynamically imports and displays all icons from the icons directory. This is the standard documentation approach for icons in this design system, as evidenced by the __storybook__/Iconography.mdx file that creates a gallery of all available icons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Storybook documentation exists for UnreadIcon
fd -e stories.tsx -e stories.mdx "UnreadIcon" packages/design-system/
Length of output: 69
Script:
#!/bin/bash
# Let's check if there are any story files in the icons directory and its structure
fd . "packages/design-system/components/icons" -t f
# Also check if other icons have stories for consistency
fd -e stories.tsx -e stories.mdx . "packages/design-system/components/icons"
Length of output: 1829
Script:
#!/bin/bash
# Let's check the content of the Iconography.mdx to understand the documentation approach
cat packages/design-system/components/icons/__storybook__/Iconography.mdx
# Also check if icons are documented in any parent storybook files
fd -e stories.tsx -e stories.mdx . packages/design-system/components --max-depth 1
Length of output: 606
packages/components/CHANGELOG.md (1)
3-4: LGTM! Version and categorization are appropriate.
The patch version increment (0.0.21) is appropriate since these changes add functionality without breaking existing features.
packages/design-system/CHANGELOG.md (1)
3-4: LGTM: Version increment follows semantic versioning.
The patch version increment is appropriate for adding new icons, as this is a non-breaking change.
packages/components/modules/comments/CommentItem/index.tsx (2)
12-12: LGTM: Import changes align with the new architecture.
The addition of ActionsOverlay and useCommentDeleteMutation imports correctly supports the migration from CommentOptions to the new overlay system.
Also applies to: 16-16
124-127: Enhance delete handler with optimistic updates and loading state.
The current implementation could benefit from optimistic updates and better loading state handling for improved user experience.
packages/components/modules/comments/Comments/__tests__/Comments.cy.tsx (1)
149-149: LGTM! Verify similar test files for consistency.
The button name changes from "delete comment" to "delete item" align with the standardization of action buttons across comments and chat rooms.
Let's verify if similar changes are needed in other test files:
Also applies to: 160-160
✅ Verification successful
Changes are isolated to this test file
The search results confirm that the button name change from "delete comment" to "delete item" is isolated to packages/components/modules/comments/Comments/__tests__/Comments.cy.tsx. No other test files in the codebase use the "delete comment" pattern, and the new "delete item" pattern is only found in this file. This is consistent with the changes being part of the standardization effort in this specific component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other test files that might need similar button name updates
# Look for test files containing "delete comment" button references
# Search in test files
rg -t tsx -t ts "delete comment" "**/__tests__/**"
# Search specifically in Cypress test files
rg -g "*.cy.{tsx,ts}" "delete comment"
Length of output: 121
Script:
#!/bin/bash
# Let's try a different approach using fd to find test files and then search within them
# First, find all Cypress test files
fd -e cy.tsx -e cy.ts | xargs rg -l "delete.*comment"
# Also search in regular test files
fd -e test.tsx -e test.ts | xargs rg -l "delete.*comment"
# Let's also search for the new pattern to understand the scope
fd -e cy.tsx -e cy.ts | xargs rg -l "delete.*item"
Length of output: 240
.../components/modules/__shared__/ActionsOverlay/__storybook__/ActionsOverlayOnButton/index.tsx
Show resolved
Hide resolved
| export const DefaultActionsOverlay: Story = { | ||
| name: 'ActionsOverlay', | ||
| args: { | ||
| title: 'Button', | ||
| enableDelete: true, | ||
| handleDeleteItem: () => {}, | ||
| offsetRight: 0, | ||
| offsetTop: 0, | ||
| ContainerProps: { | ||
| sx: { width: '100px' }, | ||
| }, | ||
| actions: [ | ||
| { | ||
| label: 'Archive', | ||
| icon: <ArchiveIcon />, | ||
| onClick: () => {}, | ||
| hasPermission: true, | ||
| closeOnClick: true, | ||
| }, | ||
| ], | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add stories demonstrating responsive behavior
The PR objectives specify that the overlay must behave consistently across all devices and screen sizes. Consider adding additional stories that demonstrate:
- Different screen sizes
- Various overlay positions
- Mobile behavior
Add stories like:
export const MobileOverlay: Story = {
name: 'Mobile View',
parameters: {
viewport: { defaultViewport: 'mobile1' },
},
args: {
// ... mobile specific args
},
}
export const TabletOverlay: Story = {
name: 'Tablet View',
parameters: {
viewport: { defaultViewport: 'tablet' },
},
args: {
// ... tablet specific args
},
}🛠️ Refactor suggestion
Add interaction tests for hover behavior
The PR objectives specify specific timing requirements for the overlay:
- Should appear within 1 second of hover
- Should disappear immediately when mouse leaves
Consider adding interaction tests to verify this behavior.
Add an interaction test:
export const HoverBehavior: Story = {
name: 'Hover Behavior',
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const button = canvas.getByRole('button');
// Test hover appearance
await userEvent.hover(button);
// Verify overlay appears within 1 second
await waitFor(() => {
expect(canvas.getByRole('menu')).toBeVisible();
}, { timeout: 1000 });
// Test hover disappearance
await userEvent.unhover(button);
// Verify immediate disappearance
expect(canvas.queryByRole('menu')).not.toBeInTheDocument();
},
};
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx
Show resolved
Hide resolved
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx
Show resolved
Hide resolved
Acceptance Criteria
Context
This story will handle the creation of the Chat List Card overlay.
Business Rules - Chat Room Overlay - 5 points
As a user, I want an option to appear on chat cards that allows me to archive, mark as unread and delete chat rooms so that I can easily identify and select chats to archive without extra steps.
When I hover my mouse over a chat list card in my Active Chat list, an overlay with an archive, mark as unread and delete appears on the card.
When I hover over a chat card, the overlay appears within one second to ensure it is quick and responsive.
Buttons are not going to be functional for now.
When I move my mouse away from the chat card, the overlay disappears immediately.
When I hover over a chat card on any device, the overlay appears consistently, regardless of screen size or resolution.
Reuse the component used in comments, that already have the behavior for mobile.
The idea of the story is to take that component and make it reusable so that it can handle the above behaviors.
Document the new component in Storybook
Guardrails
You are not expected to actually build the archive function in this story, only build the overlay behavior.
Current behavior
Expected behavior
Code Snippet
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3307-228731&node-type=frame&t=62koKJbgByOTvcDm-0
Approvd
https://app.approvd.io/projects/BA/stories/36467
Summary by CodeRabbit
Release Notes for Version 0.0.21
New Features
ActionsOverlaycomponent for managing actions related to comments and chat rooms, enhancing user interaction.ActionsOverlayinto bothCommentItemandChatRoomItemcomponents for consistent action management.ArchiveandUnreadto the design system.Bug Fixes
Documentation
ActionsOverlaycomponent in Storybook.Chores