feat(web): Generic list - External url field #16348
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libs/cms/src/lib/models/genericListItem.model.ts (2)
34-36: LGTM! Consider adding a JSDoc comment for consistency.The addition of the
externalUrlfield aligns well with the PR objectives and follows TypeScript best practices. It's correctly defined as nullable, which is appropriate for optional data.For consistency with other fields in the class, consider adding a JSDoc comment to describe the purpose of this field. For example:
/** * The URL to an external resource related to this list item. */ @Field(() => String, { nullable: true }) externalUrl?: string
68-68: LGTM! Consider using a consistent approach for default values.The addition of the
externalUrlfield to the mapped object is correct and aligns with the PR objectives. The use of optional chaining and nullish coalescing is a good practice for handling potentially undefined values.For consistency with how other fields are handled (e.g.,
title), consider using the??operator instead of relying on the truthiness of the value. This would make the code more explicit:externalUrl: fields.externalLink?.fields?.url ?? '',This change ensures that falsy values like
nullorundefinedare also replaced with an empty string, maintaining consistency with other field mappings.apps/web/components/GenericList/GenericList.tsx (1)
130-138: LGTM: Improved icon and link logicThe introduction of the
iconvariable and the updated logic for determining icons and href values enhance the component's flexibility and maintainability. This change aligns well with best practices for reusable components.Consider using a switch statement or object literal for icon mapping if more icon types are expected in the future:
const iconMap = { assetUrl: 'document', externalUrl: 'open', } as const; const icon = Object.entries(iconMap).find(([key]) => item[key as keyof typeof item])?.[1] || null;This approach would be more scalable if additional URL types and corresponding icons are added in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
- apps/web/components/GenericList/GenericList.tsx (3 hunks)
- apps/web/screens/queries/GenericList.ts (1 hunks)
- libs/cms/src/lib/models/genericListItem.model.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/components/GenericList/GenericList.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/queries/GenericList.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/cms/src/lib/models/genericListItem.model.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."
🔇 Additional comments (4)
apps/web/screens/queries/GenericList.ts (1)
27-27: LGTM! Consider updating related TypeScript types.The addition of the
externalUrlfield to theGET_GENERIC_LIST_ITEMS_QUERYaligns well with the PR objective of enabling generic list items to link to external URLs. This change enhances the flexibility of the generic list component, allowing for better integration with externally hosted content.To ensure type safety, please verify that the corresponding TypeScript types or interfaces have been updated to include this new field. Run the following script to check for potential type definitions that might need updating:
If any types are found, please ensure they include the
externalUrlfield to maintain type consistency across the application.apps/web/components/GenericList/GenericList.tsx (3)
19-19: LGTM: Improved type safety for icon propertiesThe addition of
IconPropsimport enhances type safety when working with icon-related properties in the component.
161-166: LGTM: Dynamic icon renderingThe update to dynamically set the
iconprop in the Icon component is consistent with the earlier logic changes and improves the component's flexibility. This change adheres to TypeScript best practices by leveraging the IconProps type.
Line range hint
130-166: Overall assessment: Improved component flexibility and type safetyThe changes to the ClickableItem component enhance its flexibility and type safety without affecting the overall structure or NextJS-specific functionality. The modifications align well with TypeScript and React best practices, improving the component's maintainability and reusability.
Key improvements:
- Introduction of a flexible icon assignment logic
- Dynamic href generation based on item properties
- Improved type safety with the use of IconProps
These changes contribute positively to the codebase and align with the PR objectives of enabling generic list items to link to external URLs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16348 +/- ##
==========================================
+ Coverage 36.73% 36.75% +0.02%
==========================================
Files 6808 6809 +1
Lines 141061 141104 +43
Branches 40217 40194 -23
==========================================
+ Hits 51817 51862 +45
+ Misses 89244 89242 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 11 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
|
kodiakhq
bot
deleted the
feature/web-generic-list-item-external-url-field
branch
Generic list - External url field
What
Why
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
ClickableItemcomponent to dynamically display icons based on item URLs.Bug Fixes
ClickableItemcomponent.Documentation
externalUrlfield in theGenericListItemclass.