feat(web): Add order by for team list #17545
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to team member ordering functionality across multiple files. The changes add an Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 5
🔭 Outside diff range comments (2)
libs/island-ui/contentful/src/lib/TeamList/TeamList.tsx (1)
Line range hint
1-24: Implement the sorting functionality.The
orderByprop is added to the interface but not utilized in the component implementation. The team members array should be sorted based on this prop before rendering.Add the sorting logic before rendering:
export const TeamList = ({ teamMembers, variant = 'card' }: TeamListProps) => { + const sortedMembers = React.useMemo(() => { + if (!teamMembers) return [] + return [...teamMembers].sort((a, b) => { + if (orderBy === 'name') { + return a.name.localeCompare(b.name) + } + return 0 // maintain original order for 'manual' + }) + }, [teamMembers, orderBy]) + if (variant === 'accordion') { - return <TeamMemberAccordionList teamMembers={teamMembers} /> + return <TeamMemberAccordionList teamMembers={sortedMembers} /> } - return <TeamMemberCardList teamMembers={teamMembers} /> + return <TeamMemberCardList teamMembers={sortedMembers} /> }Also applies to: 27-199
libs/cms/src/lib/cms.elasticsearch.service.ts (1)
Line range hint
605-614: Add input validation for orderBy.The method should validate the
orderByvalue to prevent runtime errors with invalid values.Add validation before processing:
async getTeamMembers( input: GetTeamMembersInput, ): Promise<TeamMemberResponse> { + if (input.orderBy && !Object.values(GetTeamMembersInputOrderBy).includes(input.orderBy)) { + throw new Error(`Invalid orderBy value: ${input.orderBy}`) + } const response = await this.getListItems({ ...input, type: 'webTeamMember', listId: input.teamListId, orderBy: input.orderBy === GetTeamMembersInputOrderBy.MANUAL ? GetGenericListItemsInputOrderBy.DATE : GetGenericListItemsInputOrderBy.TITLE, })
🧹 Nitpick comments (4)
libs/cms/src/lib/models/teamList.model.ts (1)
64-69: Simplify the mapOrderBy function.The function can be simplified by directly returning the comparison result.
- const mapOrderBy = (orderBy?: ITeamListFields['orderBy']) => { - if (orderBy === GetTeamMembersInputOrderBy.MANUAL) { - return GetTeamMembersInputOrderBy.MANUAL - } - return GetTeamMembersInputOrderBy.NAME - } + const mapOrderBy = (orderBy?: ITeamListFields['orderBy']) => + orderBy === GetTeamMembersInputOrderBy.MANUAL + ? GetTeamMembersInputOrderBy.MANUAL + : GetTeamMembersInputOrderBy.NAMElibs/cms/src/lib/search/importers/teamList.service.ts (1)
29-29: Make the counter's purpose more explicit.The counter is used for maintaining manual order, but its purpose isn't immediately clear. Consider renaming and adding a comment.
- let counter = teamList.teamMembers?.length ?? 0 + // Use descending order index for manual sorting + let manualOrderIndex = teamList.teamMembers?.length ?? 0 - releaseDate: String(counter--), + releaseDate: String(manualOrderIndex--),Also applies to: 66-66
libs/cms/src/lib/cms.elasticsearch.service.ts (1)
609-612: Simplify the orderBy mapping logic.The current implementation maps
MANUALtoDATEand everything else toTITLE. This might lead to unexpected behavior when new order types are added.Consider making the mapping more explicit:
- orderBy: - input.orderBy === GetTeamMembersInputOrderBy.MANUAL - ? GetGenericListItemsInputOrderBy.DATE - : GetGenericListItemsInputOrderBy.TITLE, + orderBy: { + [GetTeamMembersInputOrderBy.MANUAL]: GetGenericListItemsInputOrderBy.DATE, + [GetTeamMembersInputOrderBy.NAME]: GetGenericListItemsInputOrderBy.TITLE, + }[input.orderBy ?? GetTeamMembersInputOrderBy.NAME],libs/cms/src/lib/cms.resolver.ts (1)
933-940: LGTM with a minor formatting note!The implementation correctly handles both the accordion variant and sorting logic. However, there's an unnecessary empty line at line 937 that could be removed for consistency.
if (teamList?.variant === 'accordion') { return [] } - // The 'accordion' variant has a search so to reduce the inital payload (since it isn't used) we simply return an empty list return teamList?.teamMemberOrder !== GetTeamMembersInputOrderBy.MANUAL ? teamList?.teamMembers?.sort(sortAlpha('name') as any) : teamList?.teamMembers
📜 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 (9)
apps/web/components/Organization/Slice/TeamListSlice/TeamListSlice.tsx(4 hunks)apps/web/screens/queries/fragments.ts(1 hunks)apps/web/utils/richText.tsx(2 hunks)libs/cms/src/lib/cms.elasticsearch.service.ts(3 hunks)libs/cms/src/lib/cms.resolver.ts(2 hunks)libs/cms/src/lib/dto/getTeamMembers.input.ts(2 hunks)libs/cms/src/lib/models/teamList.model.ts(3 hunks)libs/cms/src/lib/search/importers/teamList.service.ts(2 hunks)libs/island-ui/contentful/src/lib/TeamList/TeamList.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
libs/island-ui/contentful/src/lib/TeamList/TeamList.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/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."
libs/cms/src/lib/search/importers/teamList.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/dto/getTeamMembers.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/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/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."
libs/cms/src/lib/cms.resolver.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/TeamListSlice/TeamListSlice.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/teamList.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 (7)
libs/cms/src/lib/dto/getTeamMembers.input.ts (1)
Line range hint
18-55: LGTM! Well-structured DTO implementation.The implementation follows best practices:
- Proper use of GraphQL decorators
- Clear type definitions
- Optional fields are properly marked
- Effective use of class-validator decorators
libs/cms/src/lib/models/teamList.model.ts (1)
23-24: LGTM! Well-structured model implementation.The implementation follows best practices:
- Proper use of decorators
- Clear type definitions
- Effective mapping logic
Also applies to: 78-78
libs/cms/src/lib/search/importers/teamList.service.ts (1)
Line range hint
29-67: LGTM! Well-structured service implementation.The implementation follows best practices:
- Proper error handling
- Clear mapping logic
- Effective use of TypeScript features
apps/web/components/Organization/Slice/TeamListSlice/TeamListSlice.tsx (1)
Line range hint
24-137: LGTM! Well-structured component implementation.The implementation follows NextJS best practices:
- Proper use of hooks
- Clear component structure
- Effective state management
- Good error handling
apps/web/utils/richText.tsx (1)
288-288: LGTM!The
orderByprop is correctly set with a fallback toGetTeamMembersInputOrderBy.Name.apps/web/screens/queries/fragments.ts (1)
297-297: LGTM!The
teamMemberOrderfield is correctly added to theTeamListFieldsfragment.libs/cms/src/lib/cms.resolver.ts (1)
140-141: LGTM!The new imports are properly organized and follow the module path conventions.
apps/web/components/Organization/Slice/TeamListSlice/TeamListSlice.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/Organization/Slice/TeamListSlice/TeamListSlice.tsx
Outdated
Show resolved
Hide resolved
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 (5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/cms/src/lib/cms.resolver.ts (1)
933-944: Good architectural choices for reusability.The implementation follows good practices for reusability:
- Uses shared utilities (
sortAlpha)- Implements clear separation of concerns
- Handles null cases defensively
🧰 Tools
🪛 GitHub Check: prepare
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/cms/src/lib/cms.resolver.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/cms/src/lib/cms.resolver.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."
🪛 GitHub Check: prepare
libs/cms/src/lib/cms.resolver.ts
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
🔇 Additional comments (3)
libs/cms/src/lib/cms.resolver.ts (3)
140-141: LGTM! Good use of shared utilities.The imports are well-organized, utilizing shared utilities for common functionality.
933-936: LGTM! Good optimization for accordion variant.The early return for the accordion variant effectively reduces the initial payload, which is a good performance optimization since the data isn't used client-side.
938-944:⚠️ Potential issueFix type mismatch in sort function.
The static analysis indicates a type mismatch where
sortAlphaexpectsItembut receivesTeamMember. This needs to be addressed for type safety.Apply this fix:
- teamMembers.sort(sortAlpha('name')) + teamMembers.sort(sortAlpha<TeamMember>('name'))Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: prepare
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
[failure] 941-941:
Argument of type '(a: Item, b: Item) => number' is not assignable to parameter of type '(a: TeamMember, b: TeamMember) => number'.
Add order by for team list
What
Why
Screenshots / Gifs
Manual order
Contentful
Web
Order by name (A-Z)
Contentful
Web
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates