feat(web): Add image to generic list item #17464
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces comprehensive changes to the Generic List functionality across multiple files. The primary focus is enhancing the list item rendering by adding image support and improving the visual presentation. The modifications span from GraphQL query updates to component rendering logic, enabling more flexible and visually rich list item displays. The changes allow conditional rendering of Changes
Sequence DiagramsequenceDiagram
participant Client
participant GenericList
participant NewsCard
participant GraphQL
Client->>GraphQL: Request list items
GraphQL-->>Client: Return items with image data
Client->>GenericList: Render list
GenericList->>GenericList: Check item image
alt Image exists
GenericList->>NewsCard: Render NewsCard
else No image
GenericList->>GenericList: Render default item
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
Finishing Touches
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: 1
🧹 Nitpick comments (5)
apps/web/screens/GenericList/GenericListItem.css.ts (1)
5-13: LGTM! Consider adding print media query.The responsive styling is well-implemented using theme utilities. The float and width values create a balanced layout.
Consider adding a print media query to ensure images are properly rendered in printed documents:
export const floatedImage = style({ ...themeUtils.responsiveStyle({ sm: { float: 'right', width: '50%', marginLeft: '16px', }, }), + '@media print': { + float: 'none', + width: '100%', + marginLeft: 0, + }, })apps/web/screens/queries/GenericList.ts (1)
52-58: Consider adding alt text field for accessibility.While the image fields are comprehensive, consider adding an
altfield for better accessibility.image { url title width height + alt } fullWidthImageInContentlibs/cms/src/lib/models/genericListItem.model.ts (1)
77-78: Consider adding validation for image dimensions.The mapping function could benefit from validation of image dimensions.
- image: fields.image ? mapImage(fields.image) : null, + image: fields.image ? validateAndMapImage(fields.image) : null, fullWidthImageInContent: fields.fullWidthImageInContent ?? true, } } + +const validateAndMapImage = (image: any) => { + const mappedImage = mapImage(image); + if (mappedImage && (!mappedImage.width || !mappedImage.height)) { + console.warn('Image dimensions missing:', mappedImage.url); + } + return mappedImage; +}apps/web/screens/GenericList/GenericListItem.tsx (1)
55-65: Add loading and error handling for images.Consider adding loading state and error handling for better user experience.
<Image {...item?.image} + loading="lazy" + onError={(e) => { + console.error('Failed to load image:', item?.image?.url); + e.currentTarget.style.display = 'none'; + }} url={ item?.image?.url ? item.image?.url + '?w=1000&fm=webp&q=80' : '' }apps/web/components/GenericList/GenericList.tsx (1)
151-170: Consider these improvements to the NewsCard implementation.
- The empty
readMoreText=""prop could be omitted since it's optional.- The date prop should be properly typed to handle undefined.
- Consider making the color values (
blue400,purple400) configurable through props for better reusability.- readMoreText="" - date={item.date ?? ''} - titleTextColor="blue400" - dateTextColor="purple400" + date={item.date} + titleTextColor={titleTextColor} + dateTextColor={dateTextColor}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
apps/web/components/GenericList/GenericList.tsx(3 hunks)apps/web/components/NewsCard/NewsCard.tsx(5 hunks)apps/web/screens/GenericList/GenericListItem.css.ts(1 hunks)apps/web/screens/GenericList/GenericListItem.tsx(3 hunks)apps/web/screens/queries/GenericList.ts(2 hunks)apps/web/screens/queries/fragments.ts(1 hunks)libs/cms/src/lib/models/genericListItem.model.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/web/screens/GenericList/GenericListItem.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/web/components/NewsCard/NewsCard.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/GenericList/GenericListItem.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/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."
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."
apps/web/screens/queries/fragments.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/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."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (8)
apps/web/screens/queries/GenericList.ts (1)
28-33: LGTM! Image fields are well-structured.The image fields include all necessary metadata for proper rendering and optimization.
libs/cms/src/lib/models/genericListItem.model.ts (1)
41-45: LGTM! Well-structured model changes.The new fields are properly decorated for GraphQL schema generation and include appropriate nullability.
apps/web/components/NewsCard/NewsCard.tsx (3)
27-27: LGTM! Well-typed prop addition.The new
titleTextColorprop is properly typed with a union type of valid color values.
41-41: LGTM! Consistent implementation.The
titleTextColorprop is consistently applied across all Text components that render the title.Also applies to: 72-72, 112-112
Line range hint
135-149: LGTM! Improved image rendering logic.The image rendering condition is properly guarded with both
showImageandimagechecks.apps/web/screens/queries/fragments.ts (1)
979-984: LGTM! Complete image field definition.The image field includes all necessary properties (url, title, width, height) for proper image rendering and optimization.
apps/web/components/GenericList/GenericList.tsx (2)
29-29: LGTM! Clean imports.The imports are properly organized and specific.
Also applies to: 36-36
138-139: LGTM! Null coalescing for image.Good use of the null coalescing operator for safe image access.
| {item.image && ( | ||
| <Box | ||
| paddingY={2} | ||
| className={cn({ | ||
| [styles.floatedImage]: item.fullWidthImageInContent !== false, | ||
| })} | ||
| > | ||
| <Image | ||
| {...item?.image} | ||
| url={ | ||
| item?.image?.url | ||
| ? item.image?.url + '?w=1000&fm=webp&q=80' | ||
| : '' | ||
| } | ||
| thumbnail={ | ||
| item?.image?.url ? item.image?.url + '?w=50&fm=webp&q=80' : '' | ||
| } | ||
| /> | ||
| </Box> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract image URL transformation to a utility function.
The image URL transformation logic should be extracted for reusability and maintainability.
Create a new utility function:
+const getOptimizedImageUrl = (url: string, options: { width: number, quality: number }) => {
+ if (!url) return '';
+ return `${url}?w=${options.width}&fm=webp&q=${options.quality}`;
+};
{item.image && (
<Box
paddingY={2}
className={cn({
[styles.floatedImage]: item.fullWidthImageInContent !== false,
})}
>
<Image
{...item?.image}
- url={
- item?.image?.url
- ? item.image?.url + '?w=1000&fm=webp&q=80'
- : ''
- }
- thumbnail={
- item?.image?.url ? item.image?.url + '?w=50&fm=webp&q=80' : ''
- }
+ url={getOptimizedImageUrl(item?.image?.url ?? '', { width: 1000, quality: 80 })}
+ thumbnail={getOptimizedImageUrl(item?.image?.url ?? '', { width: 50, quality: 80 })}
/>
</Box>
)}Committable suggestion skipped: line range outside the PR's diff.
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 9 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17464 +/- ##
==========================================
- Coverage 35.66% 35.65% -0.02%
==========================================
Files 6941 6946 +5
Lines 149144 149253 +109
Branches 42650 42678 +28
==========================================
+ Hits 53193 53211 +18
- Misses 95951 96042 +91
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Add image to generic list item
What
Why
Screenshots / Gifs
Before (Generic list item view)
After (Generic list item view)
Before (Generic list overview)
After (Generic list overview)
Checklist:
Summary by CodeRabbit
New Features
Improvements
Technical Updates