Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ui): add info card #16931

Merged
merged 53 commits into from
Dec 17, 2024
Merged

feat(ui): add info card #16931

merged 53 commits into from
Dec 17, 2024

Conversation

thorkellmani
Copy link
Member

@thorkellmani thorkellmani commented Nov 19, 2024

What

New <InfoCard /> component for UI

Screenshots / Gifs

Attach Screenshots / Gifs to help reviewers understand the scope of the pull request

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new InfoCardGrid component for displaying information cards.
    • Added toggle functionality for grid and list views in the SearchResultsContent component.
    • Enhanced pagination in the SearchResults component with a new Pagination feature.
  • Bug Fixes

    • Improved error handling for date formatting and grant status processing.
  • Documentation

    • Updated message definitions for better localization in the Grants section.
  • Refactor

    • Streamlined the mapGrant function for better clarity and maintainability.
    • Replaced the PlazaCard component with InfoCardGrid in relevant areas.
  • Style

    • Added new CSS styles for the InfoCard components to ensure consistent layout and responsiveness.

Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

Walkthrough

This pull request introduces significant changes to the grants and components system, primarily focusing on replacing the PlazaCard component with a new InfoCardGrid component. The modifications span multiple files across different modules, including updates to component styling, search results rendering, status handling, and query management. The changes enhance the flexibility of grant display, improve internationalization, and refine the data processing for grant-related information.

Changes

File Path Change Summary
.github/CODEOWNERS Removed ownership for /apps/web/components/PlazaCard/
apps/web/components/real.ts Removed PlazaCard export, added InfoCardGrid export
apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx Added grid layout toggle, replaced PlazaCard with InfoCardGrid
apps/web/screens/Grants/messages.ts Added new messages for grid/list display
apps/web/screens/Grants/utils.ts Updated status generation and parsing functions
libs/cms/src/lib/models/grant.model.ts Expanded GrantStatus enum, refined grant mapping
libs/island-ui/core/src/index.ts Added InfoCardGrid export
libs/island-ui/core/src/lib/InfoCardGrid/* New components and styling for InfoCardGrid

Sequence Diagram

sequenceDiagram
    participant User
    participant SearchResults
    participant InfoCardGrid
    participant GrantService
    
    User->>SearchResults: Perform grant search
    SearchResults->>GrantService: Fetch grants
    GrantService-->>SearchResults: Return grant data
    SearchResults->>InfoCardGrid: Render grants
    InfoCardGrid-->>SearchResults: Display grid/list view
Loading

Possibly Related PRs

Suggested Reviewers

  • robertaandersen
  • jonnigs
  • Toti91

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 3.59712% with 134 lines in your changes missing coverage. Please review.

Project coverage is 35.72%. Comparing base (c07421f) to head (a08e450).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eens/Grants/SearchResults/SearchResultsContent.tsx 0.00% 33 Missing ⚠️
...web/screens/Grants/SearchResults/SearchResults.tsx 0.00% 20 Missing ⚠️
...-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx 4.76% 20 Missing ⚠️
apps/web/screens/Grants/utils.ts 0.00% 18 Missing ⚠️
...s/island-ui/core/src/lib/InfoCardGrid/InfoCard.tsx 6.25% 15 Missing ⚠️
...nd-ui/core/src/lib/InfoCardGrid/SimpleInfoCard.tsx 11.76% 15 Missing ⚠️
...land-ui/core/src/lib/InfoCardGrid/InfoCardGrid.tsx 7.69% 12 Missing ⚠️
...ibs/cms/src/lib/search/importers/grants.service.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16931      +/-   ##
==========================================
- Coverage   35.73%   35.72%   -0.02%     
==========================================
  Files        6941     6944       +3     
  Lines      148408   148493      +85     
  Branches    42341    42381      +40     
==========================================
+ Hits        53037    53042       +5     
- Misses      95371    95451      +80     
Flag Coverage Δ
air-discount-scheme-web 0.00% <ø> (ø)
api 3.33% <ø> (ø)
api-domains-auth-admin 48.49% <ø> (ø)
api-domains-communications 39.42% <0.00%> (+<0.01%) ⬆️
application-api-files 61.87% <ø> (ø)
application-core 75.78% <ø> (ø)
application-system-api 38.74% <0.00%> (-0.01%) ⬇️
application-template-api-modules 27.83% <0.00%> (+0.02%) ⬆️
application-templates-accident-notification 28.82% <ø> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 25.77% <ø> (ø)
application-templates-driving-license 18.16% <ø> (ø)
application-templates-estate 13.69% <7.46%> (-0.07%) ⬇️
application-templates-example-payment 24.72% <ø> (ø)
application-templates-financial-aid 14.46% <ø> (ø)
application-templates-general-petition 23.15% <ø> (ø)
application-templates-inheritance-report 6.59% <ø> (ø)
application-templates-marriage-conditions 15.19% <ø> (ø)
application-templates-mortgage-certificate 43.64% <ø> (ø)
application-templates-parental-leave 29.94% <ø> (ø)
application-types 6.51% <ø> (ø)
application-ui-components 1.22% <ø> (ø)
application-ui-shell 22.32% <7.46%> (-0.15%) ⬇️
clients-charge-fjs-v2 28.35% <ø> (ø)
cms 0.39% <0.00%> (+<0.01%) ⬆️
cms-translations 38.75% <0.00%> (+<0.01%) ⬆️
contentful-apps 4.72% <ø> (ø)
financial-aid-backend 51.40% <ø> (ø)
financial-aid-shared 17.88% <ø> (ø)
island-ui-core 30.40% <0.00%> (-0.52%) ⬇️
judicial-system-api 20.21% <ø> (ø)
judicial-system-backend 55.91% <0.00%> (+<0.01%) ⬆️
judicial-system-web 27.77% <7.46%> (-0.11%) ⬇️
portals-admin-regulations-admin 1.80% <ø> (ø)
portals-core 19.61% <7.46%> (-0.23%) ⬇️
services-user-notification 46.54% <0.00%> (+<0.01%) ⬆️
shared-components 29.64% <7.46%> (-0.42%) ⬇️
shared-form-fields 33.30% <7.46%> (-0.58%) ⬇️
web 2.41% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
apps/web/screens/Grants/Grant/GrantSidebar.tsx 0.00% <ø> (ø)
apps/web/screens/Grants/messages.ts 0.00% <ø> (ø)
apps/web/screens/queries/Grants.ts 0.00% <ø> (ø)
libs/cms/src/lib/cms.elasticsearch.service.ts 2.89% <ø> (ø)
libs/cms/src/lib/models/grant.model.ts 43.26% <ø> (ø)
...ibs/cms/src/lib/search/importers/grants.service.ts 11.47% <0.00%> (+0.36%) ⬆️
...land-ui/core/src/lib/InfoCardGrid/InfoCardGrid.tsx 7.69% <7.69%> (ø)
...s/island-ui/core/src/lib/InfoCardGrid/InfoCard.tsx 6.25% <6.25%> (ø)
...nd-ui/core/src/lib/InfoCardGrid/SimpleInfoCard.tsx 11.76% <11.76%> (ø)
apps/web/screens/Grants/utils.ts 0.00% <0.00%> (ø)
... and 3 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c07421f...a08e450. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Nov 19, 2024

Datadog Report

All test runs f2b5571 🔗

38 Total Test Services: 0 Failed, 35 Passed
🔻 Test Sessions change in coverage: 8 decreased, 163 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-web 0 0 0 2 0 6.55s 1 no change Link
api 0 0 0 4 0 3.33s 1 no change Link
api-domains-auth-admin 0 0 0 18 0 13.89s 1 no change Link
api-domains-communications 0 0 0 5 0 35.42s 1 no change Link
api-domains-license-service 0 0 0 0 0 542.17ms 1 no change Link
application-api-files 0 0 0 2 0 5.58s 1 no change Link
application-core 0 0 0 97 0 22.62s 1 no change Link
application-system-api 0 0 0 46 0 2m 48.04s 1 no change Link
application-template-api-modules 0 0 0 118 0 2m 49.41s 1 no change Link
application-templates-accident-notification 0 0 0 148 0 20.25s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (8)

This report shows up to 5 code coverage decreases.

  • shared-form-fields - jest 42.01% (-0.86%) - Details
  • island-ui-core - jest 31.28% (-0.61%) - Details
  • shared-components - jest 37.03% (-0.58%) - Details
  • portals-core - jest 29.01% (-0.39%) - Details
  • application-ui-shell - jest 29.19% (-0.2%) - Details

@thorkellmani thorkellmani added the automerge Merge this PR as soon as all checks pass label Dec 4, 2024
@thorkellmani thorkellmani marked this pull request as ready for review December 4, 2024 15:19
@thorkellmani thorkellmani requested review from a team as code owners December 4, 2024 15:19
@thorkellmani thorkellmani added the deploy-feature Deploys features to dev label Dec 4, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (7)
apps/web/screens/Grants/utils.ts (2)

41-43: Refactor repeated date handling pattern

The date formatting logic is repeated across multiple case blocks with slight variations. Additionally, the potential undefined return from formatDate isn't explicitly handled.

Consider extracting the common pattern:

const formatGrantDate = (
  date: string | null | undefined,
  locale: Locale,
  format?: string
): string | undefined => {
  if (!date) return undefined;
  return formatDate(new Date(date), locale, format);
}

Then use it in the case blocks:

 case GrantStatus.Closed: {
-  const date = grant.dateTo
-    ? formatDate(new Date(grant.dateTo), locale)
-    : undefined
+  const date = formatGrantDate(grant.dateTo, locale)
   // ... rest of the code
 }

Also applies to: 60-62, 74-76, 102-104


Line range hint 134-148: Enhance type safety in generateStatusTag

While the function is well-implemented, the type safety could be improved.

Consider these type improvements:

+type StatusVariant = 'mint' | 'rose';
+interface StatusTag {
+  label: string;
+  variant: StatusVariant;
+}

 export const generateStatusTag = (
   status: Status['applicationStatus'],
   formatMessage: IntlShape['formatMessage'],
-) =>
+): StatusTag | undefined =>
   status !== 'unknown'
     ? {
         label:
           status === 'open'
             ? formatMessage(m.search.applicationOpen)
             : formatMessage(m.search.applicationClosed),
-        variant: status === 'open' ? ('mint' as const) : ('rose' as const),
+        variant: status === 'open' ? 'mint' : 'rose',
       }
     : undefined
apps/web/components/InfoCardGrid/DetailedInfoCard.tsx (2)

24-28: Enforce maximum detail lines at type level

Instead of relying on a comment to indicate the maximum number of detail lines, consider enforcing this constraint at the type level using TypeScript's tuple types.

- //max 5 lines
- detailLines?: Array<{
+ detailLines?: [
+   { icon: IconMapIcon; text: string },
+   { icon: IconMapIcon; text: string }?,
+   { icon: IconMapIcon; text: string }?,
+   { icon: IconMapIcon; text: string }?,
+   { icon: IconMapIcon; text: string }?
+ ] | {
    icon: IconMapIcon
    text: string
- }>
+ }[]

Line range hint 43-170: Consider memoizing render functions for performance optimization

The component contains several render functions that could benefit from memoization to prevent unnecessary re-renders, especially for the logo and details sections which depend on props that may not change frequently.

+ const MemoizedLogo = React.memo(({ logo, logoAlt }: { logo?: string; logoAlt?: string }) => {
+   if (!logo) return null;
+   return (
+     <Box style={{ flex: '0 0 40px' }}>
+       <img height={40} src={logo} alt={logoAlt} />
+     </Box>
+   );
+ });

+ const MemoizedDetailLine = React.memo(({ icon, text }: { icon: IconMapIcon; text: string }) => (
+   <Box display="flex" flexDirection="row" alignItems="center">
+     <Icon icon={icon} size="medium" type="outline" color="blue400" useStroke />
+     <Box marginLeft={2}>
+       <Text variant="medium">{text}</Text>
+     </Box>
+   </Box>
+ ));
apps/web/components/InfoCardGrid/InfoCard.tsx (2)

31-40: Optimize window size effect

The effect can be simplified and made more performant.

-  useEffect(() => {
-    if (width < theme.breakpoints.md) {
-      return setIsMobile(true)
-    }
-    setIsMobile(false)
-  }, [width])
+  useEffect(() => {
+    setIsMobile(width < theme.breakpoints.md)
+  }, [width])

44-71: Consider extracting style logic

The className determination logic could be extracted to improve readability.

+  const getCardStyle = (size: 'large' | 'medium' | 'small') => {
+    switch(size) {
+      case 'large': return styles.infoCardWide
+      case 'small': return styles.infoCardSmall
+      default: return styles.infoCard
+    }
+  }

   return (
     <FocusableBox
-      className={
-        size === 'large'
-          ? styles.infoCardWide
-          : size === 'small'
-          ? styles.infoCardSmall
-          : styles.infoCard
-      }
+      className={getCardStyle(size)}
apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (1)

112-113: Address TODO comment

There's an unresolved TODO comment regarding text readiness.

Would you like me to help implement the deadline status text logic or create a GitHub issue to track this task?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 988622d and eb12523.

📒 Files selected for processing (8)
  • .github/CODEOWNERS (1 hunks)
  • apps/web/components/InfoCardGrid/DetailedInfoCard.tsx (4 hunks)
  • apps/web/components/InfoCardGrid/InfoCard.tsx (1 hunks)
  • apps/web/components/InfoCardGrid/InfoCardGrid.tsx (1 hunks)
  • apps/web/components/InfoCardGrid/SimpleInfoCard.tsx (1 hunks)
  • apps/web/screens/Grants/Grant/Grant.tsx (0 hunks)
  • apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (2 hunks)
  • apps/web/screens/Grants/utils.ts (4 hunks)
💤 Files with no reviewable changes (1)
  • apps/web/screens/Grants/Grant/Grant.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/CODEOWNERS
  • apps/web/components/InfoCardGrid/InfoCardGrid.tsx
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/components/InfoCardGrid/SimpleInfoCard.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/SearchResults/SearchResultsContent.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/components/InfoCardGrid/InfoCard.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/components/InfoCardGrid/DetailedInfoCard.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/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."
🔇 Additional comments (8)
apps/web/screens/Grants/utils.ts (1)

Line range hint 11-16: Well-designed type definitions

The Status interface is well-structured with proper type constraints and optional properties. The use of literal types for applicationStatus provides excellent type safety.

apps/web/components/InfoCardGrid/DetailedInfoCard.tsx (3)

32-42: LGTM! Clean component declaration with proper typing

The component declaration follows React best practices with proper prop typing and default values.


172-180: LGTM! Clean and structured render method

The main render method follows React best practices with proper component composition and layout structure.


Line range hint 1-180: Verify component organization follows NextJS conventions

Let's verify that the component follows NextJS conventions for component organization and is properly exported.

✅ Verification successful

Let me gather more information about the component organization to ensure a thorough verification.


Component follows NextJS conventions but lacks barrel exports

The component follows NextJS conventions by being:

  • Located in the appropriate components directory structure
  • Properly organized with related components in a feature-specific folder (InfoCardGrid)
  • Using TypeScript and following React functional component patterns

However, there's a minor improvement opportunity:

  • Missing index.ts barrel file in the InfoCardGrid directory for cleaner exports
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify component organization and exports

# Check if the component is properly exported in the barrel file
rg -l "export.*DetailedInfoCard" apps/web/components/

# Check component organization
fd -t d "components" apps/web/ -x echo "Found components directory: {}"

Length of output: 726


Script:

#!/bin/bash
# Check the component's directory structure and related files
fd . apps/web/components/InfoCardGrid/ -x echo "File in InfoCardGrid: {}"

# Check if this component is being used across the application
rg "DetailedInfoCard" apps/web/ -l

# Check if there are any index files that might be exporting this component
rg "DetailedInfoCard" apps/web/components/InfoCardGrid/index.ts

Length of output: 717

apps/web/components/InfoCardGrid/SimpleInfoCard.tsx (2)

19-36: Improve header accessibility

The header section's accessibility can be enhanced by adding an appropriate aria-level attribute.


38-69: 🛠️ Refactor suggestion

Optimize content rendering logic

The content rendering logic can be simplified by extracting common elements and reducing duplication.

-  const renderContent = () => {
-    if (size === 'large') {
-      return (
-        <GridContainer>
-          <GridRow direction="row">
-            <GridColumn span="12/12">
-              <Text variant="h3" color="blue400">
-                {title}
-              </Text>
-              {description && (
-                <Box flexGrow={1} marginTop={1}>
-                  <Text>{description}</Text>
-                </Box>
-              )}
-            </GridColumn>
-          </GridRow>
-        </GridContainer>
-      )
-    }
+  const Content = () => (
+    <>
+      <Text variant="h3" color="blue400">
+        {title}
+      </Text>
+      {description && (
+        <Box marginTop={1} flexGrow={size === 'large' ? 1 : undefined}>
+          <Text>{description}</Text>
+        </Box>
+      )}
+    </>
+  )
+
+  const renderContent = () => (
+    size === 'large' ? (
+      <GridContainer>
+        <GridRow>
+          <GridColumn span="12/12">
+            <Content />
+          </GridColumn>
+        </GridRow>
+      </GridContainer>
+    ) : <Content />
+  )

Likely invalid or redundant comment.

apps/web/components/InfoCardGrid/InfoCard.tsx (1)

1-2: Optimize SSR compatibility

The useWindowSize hook might cause hydration mismatches during SSR.

apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (1)

62-128: 🛠️ Refactor suggestion

Optimize grant data transformation

The grant mapping logic is complex and could benefit from optimization.

  1. Extract the mapping logic to a separate utility function
  2. Memoize the transformed data
  3. Add proper type safety
+ const transformGrant = useCallback((grant: Grant) => {
+   if (!grant?.applicationId) return null;
+   
+   const status = parseStatus(grant, formatMessage, locale);
+   
+   return {
+     id: grant.id,
+     eyebrow: grant.fund?.title ?? grant.name ?? '',
+     // ... rest of the transformation
+   };
+ }, [formatMessage, locale]);
+ 
+ const transformedGrants = useMemo(() => 
+   grants?.map(transformGrant).filter(isDefined) ?? [],
+   [grants, transformGrant]
+ );

   return (
     <InfoCardGrid
       columns={!isGridLayout ? 1 : 2}
       variant="detailed"
-      cards={grants?.map((grant) => {
-        // ... current transformation logic
-      }).filter(isDefined) ?? []}
+      cards={transformedGrants}
     />
   );

Likely invalid or redundant comment.

apps/web/screens/Grants/utils.ts Show resolved Hide resolved
@thorkellmani thorkellmani requested a review from a team as a code owner December 16, 2024 11:36
@thorkellmani thorkellmani marked this pull request as draft December 16, 2024 11:40
@thorkellmani thorkellmani marked this pull request as ready for review December 16, 2024 14:41
Copy link
Member

@disaerna disaerna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@disaerna disaerna removed the request for review from sigruntg December 16, 2024 14:50
@thorkellmani thorkellmani added the automerge Merge this PR as soon as all checks pass label Dec 16, 2024
@thorkellmani thorkellmani changed the title feat(web): add info card feat(ui): add info card Dec 16, 2024
Copy link
Member

@brynjarorng brynjarorng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (1)
apps/web/screens/Grants/SearchResults/SearchResults.tsx (1)

Line range hint 161-169: Enhance error handling strategy

Consider implementing a more robust error handling approach:

  1. Add error state management
  2. Display user-friendly error messages
  3. Consider implementing error boundaries
+ const [error, setError] = useState<Error | null>(null);
  if (res.error) {
    setGrants([])
-   console.error('Error fetching grants', res.error)
+   setError(res.error)
+   // Consider using a toast or error message component
  }
🧹 Nitpick comments (9)
apps/web/screens/Grants/SearchResults/SearchResults.tsx (1)

Line range hint 100-106: Consider adding page number validation

While setting a default page to 1 is good, consider adding validation to ensure the parsed page number is positive and within valid bounds.

-      page: page ? Number.parseInt(page) : 1,
+      page: page ? Math.max(1, Number.parseInt(page)) || 1 : 1,
libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (2)

21-22: Ensure logoAlt prop is provided when logo is used for accessibility

To improve accessibility, the logoAlt prop should be required whenever a logo is provided. This ensures that screen readers have appropriate alternative text for images.

Consider updating the DetailedProps type to reflect this requirement:

export type DetailedProps = BaseProps & {
  logo?: string
- logoAlt?: string
+ logoAlt?: string // Consider making this required when `logo` is provided
  subEyebrow?: string
  // max 5 lines
  detailLines?: Array<{
    icon: IconMapIcon
    text: string
  }>
  tags?: Array<ActionCardProps['tag']>
}

You might enforce this at runtime or explore conditional types in TypeScript to make logoAlt required when logo is present.


87-106: Simplify renderTags by removing redundant null checks

In the renderTags function, the null check within the map function is redundant since you're already filtering out undefined values using .filter(isDefined). Simplifying the code enhances readability.

Apply this diff to streamline the tag rendering:

const renderTags = () => {
  if (!tags?.length) {
    return null
  }

  return (
    <Inline space={1}>
-     {tags
-       .map((tag, index) => {
-         if (!tag) {
-           return null
-         }
-         return (
-           <Tag key={`${tag.label}-${index}`} disabled variant={tag.variant}>
-             {tag.label}
-           </Tag>
-         )
-       })
-       .filter(isDefined)}
+     {tags
+       .filter(isDefined)
+       .map((tag, index) => (
+         <Tag key={`${tag.label}-${index}`} disabled variant={tag.variant}>
+           {tag.label}
+         </Tag>
+       ))}
    </Inline>
  )
}
libs/island-ui/core/src/lib/InfoCardGrid/InfoCardGrid.tsx (1)

18-26: Optimize isMobile state management

The isMobile state is being updated on every window resize, which could lead to performance issues. Consider using a memoized value or updating the state only when the breakpoint is crossed.

You can modify the useEffect hook:

useEffect(() => {
-   if (width < theme.breakpoints.md) {
-     return setIsMobile(true)
-   }
-   setIsMobile(false)
+   const mobile = width < theme.breakpoints.md
+   if (mobile !== isMobile) {
+     setIsMobile(mobile)
+   }
}, [width])
libs/island-ui/core/src/lib/InfoCardGrid/SimpleInfoCard.tsx (1)

14-16: Return null instead of undefined when rendering nothing

In the renderHeader function, returning null explicitly is preferred over undefined for clarity and to align with React best practices.

Apply this diff:

const renderHeader = () => {
  if (!eyebrow) {
-   return
+   return null
  }
  // ...
}
libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.tsx (2)

23-29: Consider making variant property required for better type safety

The variant property is optional for the simple type but required for detailed type. This asymmetry could lead to ambiguous type checking.

export type InfoCardProps =
  | (BaseProps & {
-     variant?: 'simple'
+     variant: 'simple'
    })
  | (DetailedProps & {
      variant: 'detailed'
    })

44-72: Add type safety for background color prop

The background color prop uses string literals which could be made type-safe.

+ type BackgroundColor = 'yellow100' | 'white';
  <FocusableBox
    ...
-   background={size === 'small' ? 'yellow100' : 'white'}
+   background={size === 'small' ? 'yellow100' as BackgroundColor : 'white' as BackgroundColor}
    ...
  >
apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (2)

30-53: Consider persisting layout preference

The layout toggle state resets on page refresh. Consider using localStorage or similar mechanism to persist user's layout preference.

- const [isGridLayout, setIsGridLayout] = useState(true)
+ const [isGridLayout, setIsGridLayout] = useState(() => {
+   if (typeof window === 'undefined') return true;
+   return localStorage.getItem('grantsLayoutPreference') !== 'list';
+ });
+ 
+ const toggleLayout = useCallback(() => {
+   const newValue = !isGridLayout;
+   setIsGridLayout(newValue);
+   localStorage.setItem('grantsLayoutPreference', newValue ? 'grid' : 'list');
+ }, [isGridLayout]);

Line range hint 128-160: Add error boundary for robust error handling

Consider wrapping the component with an error boundary to gracefully handle runtime errors.

class GrantsErrorBoundary extends React.Component {
  static getDerivedStateFromError(error) {
    return { hasError: true };
  }
  
  render() {
    if (this.state.hasError) {
      return <Box>
        <Text variant="h3">{formatMessage(m.error.somethingWentWrong)}</Text>
      </Box>;
    }
    return this.props.children;
  }
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb12523 and 2ce88fa.

📒 Files selected for processing (17)
  • .github/CODEOWNERS (0 hunks)
  • apps/web/components/real.ts (0 hunks)
  • apps/web/screens/Grants/Grant/GrantSidebar.tsx (0 hunks)
  • apps/web/screens/Grants/SearchResults/SearchResults.tsx (13 hunks)
  • apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (2 hunks)
  • apps/web/screens/Grants/messages.ts (1 hunks)
  • apps/web/screens/queries/Grants.ts (1 hunks)
  • libs/cms/src/lib/cms.elasticsearch.service.ts (0 hunks)
  • libs/cms/src/lib/models/grant.model.ts (0 hunks)
  • libs/cms/src/lib/search/importers/grants.service.ts (1 hunks)
  • libs/island-ui/core/src/index.ts (1 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (4 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts (1 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.tsx (1 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/InfoCardGrid.tsx (1 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/SimpleInfoCard.tsx (1 hunks)
  • libs/island-ui/core/src/lib/InfoCardGrid/index.ts (1 hunks)
💤 Files with no reviewable changes (5)
  • apps/web/components/real.ts
  • .github/CODEOWNERS
  • apps/web/screens/Grants/Grant/GrantSidebar.tsx
  • libs/cms/src/lib/cms.elasticsearch.service.ts
  • libs/cms/src/lib/models/grant.model.ts
✅ Files skipped from review due to trivial changes (1)
  • libs/island-ui/core/src/lib/InfoCardGrid/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/web/screens/queries/Grants.ts
  • apps/web/screens/Grants/messages.ts
  • libs/cms/src/lib/search/importers/grants.service.ts
🧰 Additional context used
📓 Path-based instructions (8)
libs/island-ui/core/src/index.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/SimpleInfoCard.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/SearchResults/SearchResultsContent.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."
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/InfoCard.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."
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."
libs/island-ui/core/src/lib/InfoCardGrid/InfoCardGrid.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/SearchResults/SearchResults.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 (12)
apps/web/screens/Grants/SearchResults/SearchResults.tsx (5)

59-60: LGTM: Good use of constant for pagination size

The PAGE_SIZE constant improves maintainability and consistency across the component.


73-78: LGTM: Improved state management with TypeScript

Good separation of concerns between grants and total hits state. The use of optional chaining with fallback values provides good type safety.


109-114: LGTM: Well-implemented pagination logic

Good use of useMemo for performance optimization and proper handling of edge cases in totalPages calculation.


309-330: LGTM: Well-implemented pagination UI

Good implementation of the Pagination component with:

  • Proper conditional rendering
  • Type-safe event handling
  • Consistent styling with variant="purple"

376-378: LGTM: Well-typed props interface

Good use of TypeScript for props definition with proper optional types.

libs/island-ui/core/src/lib/InfoCardGrid/DetailedInfoCard.tsx (1)

72-76: Verify that the updated icon size aligns with design guidelines

The icon size in renderDetails has been changed from "small" to "medium". Please ensure that this change conforms to the design specifications and maintains the intended UI consistency.

libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.css.ts (1)

3-20: Styles are well-defined and follow best practices

The CSS styles for the info cards are appropriately defined using vanilla-extract. The use of maxWidth and minHeight provides consistent sizing across different components.

libs/island-ui/core/src/lib/InfoCardGrid/InfoCardGrid.tsx (1)

1-50: Component adheres to coding guidelines

The InfoCardGrid component is well-structured, using TypeScript effectively to define props and types. It promotes reusability across NextJS applications and considers responsive design principles.

libs/island-ui/core/src/lib/InfoCardGrid/SimpleInfoCard.tsx (1)

1-71: Component is well-designed and meets the guidelines

The SimpleInfoCard component is implemented properly, with clear separation of concerns and effective use of TypeScript for prop definitions. It enhances reusability and maintainability.

libs/island-ui/core/src/lib/InfoCardGrid/InfoCard.tsx (1)

11-21: Well-structured type definitions with proper TypeScript usage

The BaseProps interface is well-defined with clear, required fields and proper TypeScript types.

libs/island-ui/core/src/index.ts (1)

63-63: LGTM: Export properly placed in Cards section

The InfoCardGrid export is correctly placed in the module exports.

apps/web/screens/Grants/SearchResults/SearchResultsContent.tsx (1)

57-126: Optimize grant data transformation

The grant mapping logic could benefit from the previously suggested optimizations regarding memoization and utility extraction.

@kodiakhq kodiakhq bot merged commit 7e8c82f into main Dec 17, 2024
248 of 252 checks passed
@kodiakhq kodiakhq bot deleted the feat/add-info-card branch December 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass deploy-feature Deploys features to dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants