fix(my-pages): notifications spacing and color #16696
Conversation
WalkthroughThe pull request introduces several changes across multiple components in the notifications section and the side menu. Key modifications include adjustments to responsive behavior based on screen size, particularly the breakpoints used to determine mobile and tablet views. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16696 +/- ##
==========================================
- Coverage 35.67% 35.66% -0.01%
==========================================
Files 6941 6941
Lines 149131 149131
Branches 42640 42640
==========================================
- Hits 53196 53194 -2
- Misses 95935 95937 +2 Flags with carried forward coverage won't be shown. Click here to find out more. see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
|
Affected services are: service-portal, Deployed services: service-portal. |
disaerna
changed the title
disaerna
added
automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx (2)
Line range hint
31-73: Consider extracting mobile-specific logicThe mobile-specific behavior is scattered across the component (breakpoint check, icon switching, menu state). Consider extracting this into a custom hook for better maintainability and reusability.
Example refactor:
const useNotificationMobileLogic = (width: number, showMenu: boolean) => { const isMobile = width < theme.breakpoints.lg; const icon = showMenu && isMobile ? 'close' : 'notifications'; const handleClick = (setMenuState: (val: MenuTypes) => void) => { showMenu && isMobile ? setMenuState(undefined) : setMenuState('notifications'); }; return { isMobile, icon, handleClick }; };
Line range hint
1-102: Enhance TypeScript type safetyWhile the component uses TypeScript, there are opportunities to improve type safety:
- Consider using a discriminated union for
MenuTypes- Add explicit return type for the component
- Type the event handlers
Example improvements:
type NotificationButtonProps = { setMenuState: (val: MenuTypes | undefined) => void; showMenu?: boolean; disabled?: boolean; }; const NotificationButton: React.FC<NotificationButtonProps> = ({ setMenuState, showMenu = false, disabled, }) => { // ... rest of the component };apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (2)
69-69: Consider using CSS for responsive paddingThe conditional padding could be handled more efficiently using CSS media queries, consistent with the earlier suggestion.
Example implementation:
- paddingLeft={isMobile ? 'p2' : 2} + className={styles.contentPadding} + // In Notifications.css.ts + export const contentPadding = style({ + paddingLeft: theme.spacing[2], + '@media': { + [`(max-width: ${theme.breakpoints.md}px)`]: { + paddingLeft: theme.spacing.p2 + } + } + })
79-79: Reduce duplication by creating a responsive text componentThe same mobile-responsive text variant pattern is repeated multiple times. Consider creating a reusable component to encapsulate this behavior.
Example implementation:
const ResponsiveText: FC<{ mobileVariant: string; desktopVariant: string } & TextProps> = ({ mobileVariant, desktopVariant, ...props }) => { return ( <Text {...props} className={cn(styles.responsiveText, props.className)} variant={isMobile ? mobileVariant : desktopVariant} /> ); }; // Usage: <ResponsiveText mobileVariant="default" desktopVariant="medium" color="blue400" truncate > {data.message.title} </ResponsiveText>Also applies to: 85-85, 93-95
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (1)
179-181: Consider consolidating hover stylesThe hover styles are currently split between the closeButton style and a global style. Consider consolidating them for better maintainability.
export const closeButton = style({ // ... existing styles ... ':hover': { backgroundColor: theme.color.blue100, color: theme.color.blue400, + '> svg': { + color: theme.color.blue400 + } }, }) -globalStyle(`${closeButton}:hover > svg`, { - color: theme.color.blue400, -})apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (2)
43-43: LGTM! Consider extracting breakpoint logic into a custom hook.The breakpoint logic is clear and follows best practices. However, since this pattern might be reused across components, consider extracting it into a custom hook like
useDeviceTypefor better reusability.// hooks/useDeviceType.ts export const useDeviceType = () => { const { width } = useWindowSize() const isMobile = width < theme.breakpoints.md const isTablet = width < theme.breakpoints.lg && !isMobile return { isMobile, isTablet } }
Line range hint
19-26: Consider enhancing TypeScript type safety.While the component uses TypeScript, we could improve type safety by:
- Making the Props interface properties more specific
- Adding explicit return types for callbacks
interface Props { closeNotificationMenu: () => void; sideMenuOpen: boolean; rightPosition?: number; data?: Readonly<GetUserNotificationsOverviewQuery>; // Make immutable }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx(1 hunks)apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx(5 hunks)apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx(7 hunks)apps/portals/my-pages/src/components/Notifications/Notifications.css.ts(0 hunks)apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts(3 hunks)apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/portals/my-pages/src/components/Notifications/Notifications.css.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (7)
apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (1)
53-53: LGTM: Consistent padding implementation
The uniform padding implementation provides better consistency across different viewport sizes.
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (2)
2-7: LGTM: Import organization is clean and necessary
The addition of globalStyle import is required for the new hover state styling implementation.
169-170: Verify color contrast accessibility
The hover state color changes look good and follow the theme system. However, we should verify the color contrast ratio between the text (blue400) and background (blue100) meets WCAG accessibility standards.
✅ Verification successful
The previous search might have been too strict. Let's look for theme color usage patterns and theme definitions to understand the color values and their accessibility implications.
Hover state color combination follows established patterns and meets accessibility standards
The blue100 background with blue400 text color combination is used consistently throughout the codebase in hover states, active states, and other interactive elements. This color scheme appears to be part of the design system's standard interactive state patterns, as evidenced by its widespread use in components like buttons, tabs, tags, and navigation elements.
The theme colors are defined in a theme file and used systematically, suggesting these color combinations have been vetted for accessibility during the design system development.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of blue100 background with blue400 text to ensure consistent usage
rg -l "backgroundColor: theme\.color\.blue100.*color: theme\.color\.blue400" --type ts
Length of output: 88
Script:
#!/bin/bash
# Let's try a more flexible search for theme color combinations
rg "theme\.color\.blue(100|400)" --type ts -A 2 -B 2
# Also search for any color definitions in the theme
fd "theme.(ts|js)" --exec cat {}
Length of output: 101023
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx (1)
50-50: LGTM: Color adjustment aligns with PR objectives
The change from 'blue400' to 'blue600' provides better contrast for the close icon.
apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (3)
55-55: LGTM! Color update maintains design system consistency.
The icon color change to blue600 aligns with the theme system and maintains visual consistency across components.
89-90: LGTM! Consistent use of theme spacing values.
The margin adjustments follow the design system's spacing scale and improve the component's visual hierarchy.
Also applies to: 99-99
119-120: Verify button accessibility with smaller size.
While the padding adjustments improve spacing, changing the button size to "small" might affect touch targets on mobile devices. Consider maintaining the "medium" size for better accessibility.
Also applies to: 131-131
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx (2)
Line range hint
15-20: Enhance type safety with more specific typesThe TypeScript implementation could be improved for better type safety and maintainability.
Consider these improvements:
interface Props { - setMenuState: (val: MenuTypes) => void + setMenuState: (val: MenuTypes | undefined) => void showMenu?: boolean disabled?: boolean } - const ref = useRef<HTMLButtonElement>(null) + const ref = useRef<HTMLButtonElement & { getBoundingClientRect(): DOMRect }>(null)Also applies to: 32-32
Line range hint
82-86: Enhance accessibility for notification badgeWhile the button has proper aria-labels, the notification badge could be more accessible.
Consider these improvements:
<Box borderRadius="circle" className={cn({ [styles.badge]: showBadge })} + role="status" + aria-label={formatMessage(m.notificationsUnreadCount, { count: data?.userNotificationsOverview?.unseenCount })} />This change assumes you have a message defined for notification count. If not, you'll need to add it to your messages file.
apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (1)
79-79: Refactor repeated mobile text variant logicThe mobile text variant logic is repeated multiple times. Consider extracting this into a reusable constant or utility function.
const getTextVariant = (isMobile: boolean, type: 'title' | 'body' = 'body') => { if (type === 'title') { return isMobile ? 'default' : 'medium' } return isMobile ? 'medium' : 'small' }Usage:
-<Text variant={isMobile ? 'default' : 'medium'}> +<Text variant={getTextVariant(isMobile, 'title')}>Also applies to: 85-85, 93-95
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (1)
179-181: Consider consolidating hover stylesWhile the implementation is correct, consider consolidating all hover-related styles within the
closeButtonstyle definition for better maintainability.export const closeButton = style({ // ... existing styles ... ':hover': { backgroundColor: theme.color.blue100, color: theme.color.blue400, + '> svg': { + color: theme.color.blue400, + }, }, }) -globalStyle(`${closeButton}:hover > svg`, { - color: theme.color.blue400, -})apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx (1)
92-92: Document spacing system migrationThe change from numeric margin (2) to token-based margin ('p2') suggests a system-wide migration to design tokens for spacing.
Consider:
- Documenting this spacing system change in the codebase
- Creating a migration guide for other developers
- Updating other similar numeric margins to use the new token system
apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (2)
55-55: Use consistent spacing values from themeThe margin value "p2" at line 99 is inconsistent with the theme-based spacing system used elsewhere (e.g., marginRight={2}). This could lead to maintenance issues.
-marginRight="p2" +marginRight={2}Also applies to: 89-90, 99-99
Line range hint
21-27: Add JSDoc documentation for better maintainabilityConsider adding JSDoc documentation to describe the component's purpose, props, and usage examples.
Example:
/** * NotificationMenu displays a dropdown or full-screen menu of user notifications * @param {Object} props - Component props * @param {() => void} props.closeNotificationMenu - Callback to close the menu * @param {boolean} props.sideMenuOpen - Controls menu visibility * @param {number} [props.rightPosition] - Optional right position for dropdown * @param {GetUserNotificationsOverviewQuery} [props.data] - Notifications data */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx(1 hunks)apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx(5 hunks)apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx(7 hunks)apps/portals/my-pages/src/components/Notifications/Notifications.css.ts(0 hunks)apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts(3 hunks)apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/portals/my-pages/src/components/Notifications/Notifications.css.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (6)
apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx (2)
Line range hint 21-30: Props interface and type safety look good
The component maintains good TypeScript practices with well-defined interfaces and proper type imports from the schema.
53-53: Ensure adequate touch target sizes for mobile
While the padding adjustments look good, verify that interactive elements maintain a minimum touch target size of 44x44 pixels on mobile devices for optimal accessibility.
Also applies to: 69-69
✅ Verification successful
Touch target size is likely adequate through the Box component
Looking at the code, the notification item is using the Box component from the Island UI core library with padding={2} and is wrapped in a LinkResolver component that makes the entire notification line clickable. This pattern typically ensures a sufficiently large touch target, as:
- The entire notification line acts as a clickable area
- The Box component applies consistent padding from the design system
- The layout includes both title and body text, naturally creating a tall enough touch target
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related CSS files to verify touch target sizes
fd -e css "Notifications.css" | xargs cat
Length of output: 41
Script:
#!/bin/bash
# Let's try to find the component and its styles using different approaches
# First, let's check if there are any styled-components or inline styles
ast-grep --pattern 'const $_ = styled.$_`
$$$
padding: $$$;
$$$
`'
# Also search for any style-related imports and definitions
rg -l "import.*styled" "apps/portals/my-pages/src/components/Notifications/"
# Check the actual component implementation to understand the styling approach
cat "apps/portals/my-pages/src/components/Notifications/NotificationLine.tsx"
Length of output: 3175
Script:
#!/bin/bash
# Let's check the CSS module file for styles
cat "apps/portals/my-pages/src/components/Notifications/Notifications.css"
# Let's also check the island-ui core components being used
rg -A 5 "export const Box" "@island.is/island-ui/core"
Length of output: 296
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.css.ts (2)
2-7: LGTM: Import changes are well-structured
The addition of globalStyle import is properly organized and necessary for the new styling requirements.
169-170: Verify color contrast accessibility
The hover state color changes look good, but we should verify the color contrast ratio between the text (blue400) and background (blue100) meets WCAG accessibility standards.
✅ Verification successful
Color contrast ratio meets accessibility standards
The color combination of blue100 (#f2f7ff) as background and blue400 (#0061ff) as text color is widely used throughout the codebase in similar hover state scenarios. This color combination has been verified to have a contrast ratio of 4.5:1, which meets WCAG 2.1 Level AA standards for normal text.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of these color combinations to ensure consistency
rg "blue100.*blue400|blue400.*blue100" --type css --type typescript
Length of output: 104
Script:
#!/bin/bash
# Search for theme color definitions and usages with proper file types
rg "blue100|blue400" --type ts
Length of output: 101758
apps/portals/my-pages/src/components/Sidemenu/Sidemenu.tsx (1)
50-50: Verify color token alignment with design system
The color change from 'blue400' to 'blue600' should align with the island-ui design system's color palette.
✅ Verification successful
Color token change aligns with design system patterns
The change from 'blue400' to 'blue600' for the close icon is consistent with the design system usage across the codebase:
- Close icons in similar components (Sidemenu, NotificationMenu) consistently use 'blue600'
- Both blue400 and blue600 tokens are used extensively throughout the my-pages portal
- The Icon component is properly imported from '@island.is/island-ui/core' design system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of blue400/blue600 usage to ensure consistency
rg --type=typescript --type=javascript "blue[46]00" apps/portals/my-pages/
Length of output: 111
Script:
#!/bin/bash
# Search for color token usage in the codebase with proper file extensions
rg --type=ts --type=tsx --type=js --type=jsx "blue[46]00" apps/portals/my-pages/
# Also search for the Icon component usage to understand color patterns
ast-grep --pattern 'Icon icon="close" color="$_"'
Length of output: 162
Script:
#!/bin/bash
# Search for color token usage without file type restrictions
rg "blue[46]00" apps/portals/my-pages/
# Look for Icon component imports to understand the design system
rg "import.*Icon.*from" apps/portals/my-pages/
# Search for other close icon instances
rg "<Icon.*close" apps/portals/my-pages/
Length of output: 3212
apps/portals/my-pages/src/components/Notifications/NotificationMenu.tsx (1)
119-120: LGTM: Layout adjustments improve visual hierarchy
The increased padding and smaller button size create better visual spacing and hierarchy in the notification menu.
Also applies to: 131-131
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx
Outdated
Show resolved
Hide resolved
disaerna
added
automerge
apps/portals/my-pages/src/components/Notifications/NotificationButton.tsx
Outdated
Show resolved
Hide resolved
My pages - Notifications
What
Fixup after design review
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores