feat(web): Grant - Mobile view and sidemenu refactor #17214
Conversation
WalkthroughThis pull request introduces enhancements to the Grants section of the web application. The changes restructure the grant details page by breaking down the previous 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
|
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 (1)
|
|
Affected services are: api,application-system-api,financial-aid-api,financial-aid-backend,financial-aid-open-api,services-auth-personal-representative,air-discount-scheme-web,consultation-portal,contentful-apps,financial-aid-web-osk,financial-aid-web-veita,judicial-system-web,skilavottord-web,web,application-system-form,island-ui-storybook,portals-admin,service-portal,system-e2e, Deployed services: application-system-api,application-system-form,service-portal,portals-admin,consultation-portal,api,web,skilavottord-web,island-ui-storybook,air-discount-scheme-web,application-system-api-worker,services-bff-portals-admin,services-bff-portals-my-pages. |
disaerna
changed the title
![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
🧹 Nitpick comments (12)
apps/web/screens/Grants/Grant/Grant.css.ts (2)
3-3: Consider adding explicit styles or renaming the styleThe empty style object with name
noUnderlineis misleading as it doesn't directly handle underlines. Consider either:
- Adding explicit styles in the object
- Renaming to better reflect its purpose (e.g.,
noBoxShadow)
5-7: Consider using a more specific selectorThe global style targeting all spans within elements using this class could be too broad. Consider using a more specific selector or moving the style to the component level if it's only used in a specific context.
-globalStyle(`${noUnderline} span`, { +globalStyle(`${noUnderline} span[class*="Button"]`, { boxShadow: 'none', })apps/web/screens/Grants/Grant/GrantSidebar/PanelLine.tsx (1)
3-15: Consider enhancing component flexibility with spacing propsThe component follows good practices but could be more flexible. Consider:
- Adding spacing props for vertical margins
- Making the Box component's props configurable
-export const generateLine = (heading: string, content?: React.ReactNode) => { +interface PanelLineProps { + heading: string; + content?: React.ReactNode; + spacing?: number; + boxProps?: BoxProps; +} + +export const generateLine = ({ + heading, + content, + spacing = 2, + boxProps +}: PanelLineProps) => { if (!content) { return null } return ( - <Box> + <Box marginY={spacing} {...boxProps}> <Text variant="medium" fontWeight="semiBold"> {heading} </Text>apps/web/screens/Grants/Grant/GrantSidebar/GrantSidebar.tsx (4)
35-54: Optimize goBackToDashboard with useMemoThe function recreates JSX on every render. Consider memoizing it with useMemo for better performance.
- const goBackToDashboard = () => { + const goBackToDashboard = useMemo(() => { return ( <Box printHidden className={styles.noUnderline}> <LinkV2 href={'/styrkjatorg/styrkir'}> <Button preTextIcon="arrowBack" preTextIconType="filled" size="small" type="button" variant="text" truncate as="span" unfocusable > {formatMessage(m.general.goBack)} </Button> </LinkV2> </Box> ) - } + }, [formatMessage])
38-38: Move hardcoded URL to constantsThe URL '/styrkjatorg/styrkir' should be moved to a constants file for better maintainability.
+const ROUTES = { + GRANTS_DASHBOARD: '/styrkjatorg/styrkir' +} as const + - <LinkV2 href={'/styrkjatorg/styrkir'}> + <LinkV2 href={ROUTES.GRANTS_DASHBOARD}>
64-66: Simplify null coalescing with nullish coalescing operatorThe current implementation could be simplified using the nullish coalescing operator.
- institution={ - grant.fund?.parentOrganization.title ?? - formatMessage(m.single.unknownInstitution) - } + institution={grant.fund?.parentOrganization.title ?? formatMessage(m.single.unknownInstitution)}
39-50: Extract button props for reusabilityConsider extracting the common button props to a constant for reusability across the application.
+const BACK_BUTTON_PROPS = { + preTextIcon: "arrowBack" as const, + preTextIconType: "filled" as const, + size: "small" as const, + type: "button" as const, + variant: "text" as const, + truncate: true, + as: "span" as const, + unfocusable: true, +} as const; - <Button - preTextIcon="arrowBack" - preTextIconType="filled" - size="small" - type="button" - variant="text" - truncate - as="span" - unfocusable - > + <Button {...BACK_BUTTON_PROPS}>apps/web/screens/Grants/Grant/GrantSidebar/ExtraPanel.tsx (1)
15-71: Consider enhancing error handling and accessibilityThe component handles null checks well but could benefit from:
- URL validation before rendering links
- Aria labels for better screen reader support
- Loading states for async resources
Consider adding these enhancements:
<LinkV2 newTab key={link.id} href={link.url} + aria-label={`Support link for ${link.text}`} underlineVisibility="hover" >apps/web/screens/Grants/Grant/GrantSidebar/DetailPanel.tsx (1)
21-114: Consider adding error boundary and loading statesThe component could benefit from error handling and loading states for better user experience.
- Wrap the component with an error boundary
- Add loading states for async data
- Handle edge cases when grant data is incomplete
Would you like me to provide an example implementation?
apps/web/screens/Grants/Grant/Grant.tsx (3)
117-138: Enhance type safety for ActionCard propsConsider creating a dedicated type for the ActionCard's cta configuration to improve maintainability and type safety.
+type ActionCardCTA = { + disabled: boolean + size: 'small' + label: string + onClick: () => void + icon: 'open' + iconType: 'outline' +} <ActionCard heading={grant.name} text={status ? generateStatusTag(status.applicationStatus, formatMessage)?.label + (status.deadlineStatus ? ' / ' + status.deadlineStatus : '') : ''} backgroundColor="blue" - cta={{ + cta={({ disabled: status?.applicationStatus === 'closed' || status?.applicationStatus === 'unknown', size: 'small', label: grant.applicationButtonLabel ?? formatMessage(m.single.apply), onClick: () => router.push(grant.applicationUrl?.slug ?? ''), icon: 'open', iconType: 'outline', - }} + } as ActionCardCTA)}
193-206: Consider grouping mobile-specific panelsThe mobile-specific panels (ExtraPanel and InstitutionPanel) could be grouped into a single component to improve maintainability and reduce the number of Hidden components.
+const MobilePanels: React.FC<{ grant: Grant; locale: Locale }> = ({ grant, locale }) => { + const { formatMessage } = useIntl() + return ( + <> + <ExtraPanel grant={grant} /> + <InstitutionPanel + institutionTitle={formatMessage(m.single.provider)} + institution={ + grant.fund?.parentOrganization.title ?? + formatMessage(m.single.unknownInstitution) + } + img={grant.fund?.parentOrganization.logo?.url} + locale={locale} + /> + </> + ) +} -<Hidden above="md"> - <ExtraPanel grant={grant} /> -</Hidden> -<Hidden above="md"> - <InstitutionPanel - institutionTitle={formatMessage(m.single.provider)} - institution={ - grant.fund?.parentOrganization.title ?? - formatMessage(m.single.unknownInstitution) - } - img={grant.fund?.parentOrganization.logo?.url} - locale={locale} - /> -</Hidden> +<Hidden above="md"> + <MobilePanels grant={grant} locale={locale} /> +</Hidden>
Line range hint
81-83: Enhance error handling with specific messagesConsider providing more specific feedback when the grant is not found, rather than returning null.
if (!grant) { - return null + return ( + <Box padding={4}> + <Text variant="h1">{formatMessage(m.errors.grantNotFound)}</Text> + <Text marginTop={2}>{formatMessage(m.errors.grantNotFoundDescription)}</Text> + </Box> + ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/screens/Grants/Grant/Grant.css.ts(1 hunks)apps/web/screens/Grants/Grant/Grant.tsx(6 hunks)apps/web/screens/Grants/Grant/GrantSidebar.tsx(0 hunks)apps/web/screens/Grants/Grant/GrantSidebar/DetailPanel.tsx(1 hunks)apps/web/screens/Grants/Grant/GrantSidebar/ExtraPanel.tsx(1 hunks)apps/web/screens/Grants/Grant/GrantSidebar/GrantSidebar.tsx(1 hunks)apps/web/screens/Grants/Grant/GrantSidebar/PanelLine.tsx(1 hunks)apps/web/screens/Grants/messages.ts(2 hunks)apps/web/screens/Grants/utils.ts(1 hunks)libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/screens/Grants/Grant/GrantSidebar.tsx
🧰 Additional context used
📓 Path-based instructions (9)
apps/web/screens/Grants/Grant/GrantSidebar/PanelLine.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/web/screens/Grants/Grant/Grant.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."
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/web/screens/Grants/messages.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/web/screens/Grants/utils.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/web/screens/Grants/Grant/GrantSidebar/ExtraPanel.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/web/screens/Grants/Grant/GrantSidebar/GrantSidebar.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/web/screens/Grants/Grant/GrantSidebar/DetailPanel.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/web/screens/Grants/Grant/Grant.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 (5)
apps/web/screens/Grants/messages.ts (2)
9-12: LGTM: New navigation message follows conventions.
The new goBack message is well-structured and follows the established naming conventions for internationalization keys.
89-89: Verify the message change impact across components.
The simplified message removes "og með" from the deadline text. While this makes the message more concise, we should verify that this change doesn't affect the semantic meaning in components using this message.
✅ Verification successful
Message change is safe to implement
The verification shows that the message is used in a conditional context where applicationWasOpenToAndWith is only selected when the date contains a time part. The removal of "og með" from the message is consistent with this logic, as it's used to display a simple deadline format. The change maintains the semantic meaning while making the text more concise.
- The message is only referenced in
utils.tswhere it's properly handled based on the date format - No direct usage of this message was found in the SearchResults components
- The change aligns with other similar deadline messages in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components using the applicationWasOpenToAndWith message
# Search for direct message usage
rg "applicationWasOpenToAndWith" --type ts --type tsx
# Search for components that might format or display application deadlines
ast-grep --pattern 'defaultMessage: $msg' apps/web/screens/Grants
Length of output: 149
Script:
#!/bin/bash
# Search for direct message usage with correct file type
rg "applicationWasOpenToAndWith" -t ts
# Search for components that might format or display application deadlines
# First, find all message definitions in the Grants directory
rg "defaultMessage.*Frestur" -t ts
# Look for imports of messages.ts to track usage
rg -l "from.*messages" apps/web/screens/Grants/
Length of output: 1556
Script:
#!/bin/bash
# Let's check the actual usage context in utils.ts
rg -B 3 -A 3 "applicationWasOpenToAndWith" apps/web/screens/Grants/utils.ts
# Check the content around the message usage in components
rg -B 3 -A 3 "Frestur.*til" apps/web/screens/Grants/SearchResults/
Length of output: 384
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
41-41: LGTM! Good improvement for mobile responsiveness
Adding flexWrap: 'wrap' is a good solution for handling long breadcrumbs on mobile devices, aligning well with the PR objectives.
Let's verify this doesn't affect existing implementations:
✅ Verification successful
Let me check the CSS files to ensure there are no conflicting styles that might affect the flex-wrap property.
Verified: The flexWrap addition is safe and won't conflict with existing styles
After reviewing the CSS files, there are no conflicting styles that would interfere with the flexWrap: 'wrap' property. The CSS files only define color schemes, focus states, and basic layout properties like margins and display types. The flex-wrap property will work as intended for improving mobile responsiveness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for custom styling that might conflict with flex-wrap
rg -g '*.{ts,tsx,css}' -A 3 'Breadcrumbs.*style|Breadcrumbs.*css'
Length of output: 1623
Script:
#!/bin/bash
# Check the content of all Breadcrumbs CSS files
fd "Breadcrumbs.css" --exec cat {}
Length of output: 3087
apps/web/screens/Grants/Grant/Grant.tsx (2)
75-78: Good use of useMemo for status computation
The memoization of status parsing prevents unnecessary recomputation when unrelated props change.
113-115: Verify mobile/desktop breakpoint behavior
The component uses different layouts for mobile (<sm) and desktop (>md) views. Please verify:
- The behavior at exactly the 'sm' and 'md' breakpoints
- That no content is hidden at both breakpoints simultaneously
- That no content is duplicated across breakpoints
Also applies to: 116-139
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts (1)
22-23: Consider removing empty style object if only used for targeting SVGs.The empty style object
iconBoxappears to be used solely as a selector for targeting SVG elements. Consider using a more direct approach or combining the styles.-export const iconBox = style({}) - -globalStyle(`${iconBox} > svg`, { - minWidth: 24, -}) +export const iconBox = style({ + '& > svg': { + minWidth: 24, + }, +})libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (1)
Line range hint
25-25: Consider extracting magic number to a named constant.The limit of 5 detail lines appears as a magic number in both the comment and implementation. Consider extracting it to a named constant for better maintainability.
+const MAX_DETAIL_LINES = 5; + export type DetailedProps = BaseProps & { logo?: string logoAlt?: string subEyebrow?: string - //max 5 lines + //max MAX_DETAIL_LINES lines detailLines?: Array<{ icon: IconMapIcon text: string }> tags?: Array<ActionCardProps['tag']> } // ... later in the code ... - {detailLines?.slice(0, 5).map((d, index) => ( + {detailLines?.slice(0, MAX_DETAIL_LINES).map((d, index) => (Also applies to: 69-69
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx(2 hunks)libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts (1)
24-26: Review mobile responsiveness of fixed SVG width.
The hard-coded minWidth: 24 for SVGs might affect the component's appearance on smaller screens. Consider using relative units or adding responsive breakpoints.
✅ Verification successful
Fixed SVG width is appropriate for this use case
The minWidth: 24 for SVGs in the InfoCard component is actually a reasonable choice:
- The component already has responsive layout handling through its parent grid system, as seen in the judicial system implementation.
- The 24px minimum width ensures icons maintain consistent and readable dimensions while allowing them to scale larger if needed.
- This is a common practice for icon sizing in design systems to maintain visual consistency.
The current implementation provides a good balance between consistency and flexibility, and doesn't require modification for mobile responsiveness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for responsive width implementations in other icon-related styles
rg -l "minWidth.*px" --type css
Length of output: 33
Script:
#!/bin/bash
# Let's check for any SVG-related styles and responsive patterns in the component
rg -g "*.ts" -g "*.tsx" "minWidth.*(?:px|rem|em|vw)" -A 2
# Also check for any media query implementations in the same file
rg -g "InfoCard.css.ts" "media|screen|@media"
# Look for other SVG styling patterns in the codebase
ast-grep --pattern 'globalStyle($_, { $$$minWidth$$$})'
Length of output: 12639
Script:
#!/bin/bash
# Let's check the specific InfoCard component and its responsive implementations
rg -g "InfoCard.css.ts" "(?:minWidth|width)" -A 2
# Check for any SVG-specific styling patterns in the same component
rg -g "InfoCard.css.ts" "svg|icon" -B 2 -A 2
Length of output: 1299
libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (1)
18-18: LGTM! Style implementation follows best practices.
The implementation:
- Uses TypeScript effectively for type safety
- Follows modular styling approach
- Maintains component reusability
Also applies to: 70-70
Yes fixed @thorkellmani |
Grant - Tweaks
What
Add subheading to action card apply button
Add "Go back" arrow
Rearrange sidemenu
Add mobile view for "sidemenu" props
Add disabled logic to button
Minor UI tweaks
Island UI -> make Breadcrumbs wrap if too long
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
DetailPanel,ExtraPanel, andInstitutionPanelfor enhanced grant information display.Improvements
ActionCardto dynamically reflect grant status and deadlines.Breadcrumbscomponent to allow items to wrap within the container.Bug Fixes
Chores
GrantSidebarcomponent to streamline the codebase.