-
Notifications
You must be signed in to change notification settings - Fork 61
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(web): Generic List - Order by field in CMS #17319
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces a new feature for ordering generic list items across multiple components and services. The changes primarily focus on adding a Changes
Sequence DiagramsequenceDiagram
participant Client
participant GenericListWrapper
participant GraphQLQuery
participant ElasticsearchService
Client->>GenericListWrapper: Render with defaultOrder
GenericListWrapper->>GraphQLQuery: Fetch items with orderBy
GraphQLQuery->>ElasticsearchService: Request sorted items
ElasticsearchService-->>GraphQLQuery: Return sorted list items
GraphQLQuery-->>GenericListWrapper: Provide ordered items
GenericListWrapper->>Client: Render ordered list
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17319 +/- ##
==========================================
- Coverage 35.69% 35.69% -0.01%
==========================================
Files 6928 6928
Lines 148718 148767 +49
Branches 42495 42520 +25
==========================================
+ Hits 53087 53098 +11
- Misses 95631 95669 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 (4) |
There was a problem hiding this 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
🔭 Outside diff range comments (1)
apps/web/components/Organization/Slice/SliceMachine.tsx (1)
Line range hint
1-300
: Consider addressing @ts-ignore commentsThe file contains multiple
@ts-ignore
comments that suppress TypeScript errors. These should be properly typed to maintain type safety.Consider creating proper type definitions for:
- The
slice
parameter in therenderSlice
function- The
slice.__typename
checks in theSliceMachine
componentExample approach:
type SliceWithTypename = Slice & { __typename: string; }; export const renderSlice = ( slice: SliceWithTypename, namespace: Record<string, string>, slug: string, params: Record<string, unknown> ) => { // ... rest of the code }
🧹 Nitpick comments (2)
libs/cms/src/lib/dto/getGenericListItems.input.ts (1)
13-17
: Enum addition for ordering.
Clearly enumerating sorting criteria as “Date”, “Title”, and “Publish Date” is beneficial for readability. Ensure your naming in the UI is intuitive to end-users.apps/web/components/GenericList/GenericList.tsx (1)
582-582
: Consider adding JSDoc documentation for the defaultOrder propTo improve code maintainability and help other developers understand the purpose of this prop, consider adding JSDoc documentation.
interface GenericListWrapperProps { id: string searchInputPlaceholder?: string | null itemType?: string | null filterTags?: GenericTag[] | null + /** Specifies the default ordering for the list items. When not provided, items will use the default backend ordering. */ defaultOrder?: GetGenericListItemsInputOrderBy | null }
📜 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.ts
is excluded by!**/generated/**
📒 Files selected for processing (9)
apps/web/components/GenericList/GenericList.tsx
(3 hunks)apps/web/components/Organization/Slice/SliceMachine.tsx
(1 hunks)apps/web/screens/queries/fragments.ts
(1 hunks)apps/web/utils/richText.tsx
(1 hunks)libs/cms/src/lib/cms.elasticsearch.service.ts
(3 hunks)libs/cms/src/lib/dto/getGenericListItems.input.ts
(2 hunks)libs/cms/src/lib/models/genericList.model.ts
(3 hunks)libs/cms/src/lib/models/latestGenericListItems.model.ts
(1 hunks)libs/cms/src/lib/search/importers/genericListItem.service.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
libs/cms/src/lib/dto/getGenericListItems.input.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/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."
apps/web/utils/richText.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/search/importers/genericListItem.service.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/cms/src/lib/models/genericList.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."
libs/cms/src/lib/models/latestGenericListItems.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/components/Organization/Slice/SliceMachine.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/cms.elasticsearch.service.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 (20)
libs/cms/src/lib/cms.elasticsearch.service.ts (6)
42-45
: Ensure maintainability of new imports.
It's great that the new enum and input type are imported here. For optimal discoverability, consider adding a brief doc comment or JSDoc remark explaining the purpose of these imports and how they enable sorting.
476-476
: Good extension of method signature.
The optional “orderBy?” parameter reflects the new sorting logic. This approach is flexible and cleanly integrates the new feature with minimal code changes.
532-533
: Sorting array initialization is sufficient.
Using an empty array to accumulate sorting rules is fine. No issues found here.
534-547
: Robust default sort logic.
Your fallback condition uses descending release date and text-based sorting in a stable manner. Consider verifying that the default logic matches expectations in upstream usage (e.g., date-based sorting might cause confusion if users consider “publish date” different from “release date”).
Would you like a script to search the codebase for references to “releaseDate” or “publish date” to confirm consistent usage?
549-559
: Title-based sort logic.
Sorting by title (ascending) is appropriate for alphabetical sorting. Looks correct and aligns well with the fallback to other fields if titles are identical.
561-571
: Publish-date-based sort logic.
Sorting by dateCreated first, then fallback by title, then releaseDate provides a clear chronological structure. Ensure that calling code expects “publish date” to be captured by “dateCreated”.
Would you like a script to locate usage references of “publish date” or “dateCreated” to finalize consistency checks?
libs/cms/src/lib/dto/getGenericListItems.input.ts (4)
1-7
: Proper usage of GraphQL decorators.
The imports from '@nestjs/graphql' are correct and well-structured. Good job keeping them consistent.
11-12
: Caching configuration is appropriate.
The import of “CacheField” indicates an efficient approach to reduce repeated field lookups.
19-21
: Enum registration with GraphQL.
Registering your enum helps keep your schema consistent. Good job making the name explicit.
55-56
: New “orderBy” field is consistent.
Allowing it to be null or omitted is a flexible approach. This neatly maps onto your new sorting logic.
libs/cms/src/lib/models/genericList.model.ts (4)
6-6
: New import aligns with sorting functionality.
Importing GetGenericListItemsInputOrderBy from the DTO is correct. This promotes cohesive usage of the enum across layers.
31-32
: Optional “defaultOrder” field.
Defining “defaultOrder” at the model level and annotating it with CacheField is a good way to provide a default sorting at the data layer.
40-51
: Mapping function is straightforward.
The map from a string value to your new enum is clear and easy to maintain. No issues detected.
62-62
: Mapping “defaultOrder” usage.
You ensure that the model’s “defaultOrder” matches what is set in Contentful fields. This keeps future maintenance simple.
libs/cms/src/lib/models/latestGenericListItems.model.ts (2)
68-72
: Concise variable extraction.
Extracting “genericList” before returning promotes readability. Nice improvement in code structure.
73-92
: Propagating “defaultOrder” to “itemResponse”.
Including “orderBy: genericList?.defaultOrder” ensures alignment between the mapped data and the new sorting capabilities.
apps/web/components/Organization/Slice/SliceMachine.tsx (1)
206-206
: LGTM! Proper prop passing for defaultOrder
The defaultOrder
prop is correctly passed from the slice to the GenericListWrapper
component.
apps/web/utils/richText.tsx (1)
278-278
: LGTM! Consistent prop handling across components
The changes correctly:
- Add the
defaultOrder
prop toGenericList
- Explicitly type the
variant
prop as 'accordion' | 'card' inTeamList
These changes maintain consistency with the modifications in SliceMachine.tsx
.
Also applies to: 283-286
apps/web/screens/queries/fragments.ts (1)
876-876
: LGTM: Field addition to GraphQL fragment
The defaultOrder
field is correctly added to the GenericListFields
fragment.
apps/web/components/GenericList/GenericList.tsx (1)
33-33
: LGTM: Implementation of defaultOrder
The changes correctly implement the default ordering functionality with proper TypeScript types and GraphQL integration.
Also applies to: 582-582, 590-590, 653-653
Generic List - Order by field in CMS
What
Why
Checklist:
Summary by CodeRabbit
New Features
defaultOrder
property for theGenericList
component, allowing for customizable item ordering.SliceMachine
to support the newdefaultOrder
functionality in rendering slices.CmsElasticsearchService
, enabling item retrieval based on various criteria.Bug Fixes
GenericListItemSyncService
by prioritizing the first published date.Documentation
defaultOrder
field in theGenericListFields
.