fix(service-portal-education): Fix typing #16148
Conversation
WalkthroughThe changes involve renaming keys in the Changes
Possibly related PRs
Suggested labels
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
🧹 Outside diff range and nitpick comments (6)
libs/service-portal/education/src/screens/SecondarySchoolCareer/SecondarySchoolCareer.tsx (4)
80-84: Approved: Consistent use of English keysThe changes from Icelandic to English keys in the
labelsobject improve consistency and align with the PR objective. This modification should help prevent build issues related to non-English characters.For even better consistency, consider using camelCase for multi-word keys:
- course: formatMessage(edMessage.courseId), + courseId: formatMessage(edMessage.courseId),This change would make the key naming consistent with JavaScript conventions and the
courseNamekey used elsewhere in the code.
90-94: Approved: Consistent key renaming in items mappingThe changes in the
itemsmapping correctly align with the modifications made to thelabelsobject. This consistency ensures that theSortableTablecomponent will continue to function as expected with the new English key names.To maintain consistency with the previous suggestion:
- course: course?.courseId ?? '', + courseId: course?.courseId ?? '',This change would align with the suggested camelCase naming convention for multi-word keys.
100-101: Approved: Consistent key renaming in footer objectThe renaming of the
brautarheitikey tocoursein thefooterobject is consistent with the previous changes and contributes to the overall goal of using English keys throughout the component.For consistency with the previous suggestions:
- course: '', + courseId: '',This change would align the
footerobject with the suggested camelCase naming convention for multi-word keys used in thelabelsanditemsobjects.
Line range hint
1-108: Overall assessment: Improved consistency and maintainabilityThe changes in this file successfully address the PR objective of replacing non-English letters in object keys. The consistent renaming of keys from Icelandic to English across the
EducationGraduationDetailcomponent improves code readability and maintainability.Key points:
- The changes align with the coding guidelines for library files.
- TypeScript usage is maintained, ensuring type safety.
- The component's reusability is preserved.
- The modifications should help prevent build issues related to non-English characters.
Consider implementing the suggested minor improvements for even better consistency in naming conventions.
To further enhance the component's reusability across different NextJS apps, consider extracting the label messages into a separate, importable constant object. This would allow for easier localization and potential reuse in other components or applications.
libs/service-portal/education/src/screens/SecondarySchoolGraduationDetail/SecondarySchoolGraduationDetail.tsx (2)
118-122: Approved: Improved naming consistencyThe renaming of object keys from Icelandic to English improves code consistency and readability. This change aligns well with the PR objective of addressing issues related to non-English letters in object keys.
Consider using an interface or type alias for the
labelsobject to enhance type safety and documentation. For example:interface TableLabels { name: string; course: string; units: string; grade: string; date: string; status: string; } const labels: TableLabels = { name: formatMessage(edMessage.courseName), course: formatMessage(edMessage.courseId), units: formatMessage(edMessage.units), grade: formatMessage(edMessage.grade), date: formatMessage(edMessage.dateShort), status: formatMessage(edMessage.status), };This would improve type checking and make the code more self-documenting.
128-132: Approved: Consistent naming in items mappingThe renaming of object keys in the
itemsmapping is consistent with the changes in thelabelsobject, improving overall code consistency and readability.To enhance type safety and make the code more robust, consider defining an interface for the course object and using it in the mapping. For example:
interface Course { courseId: string; courseName: string; units: string; finalgrade: string; date: string; status: string; } // In the items mapping items={ item.courses?.map((course: Course, i) => ({ id: course.courseId ?? `${i}`, name: course.courseName ?? '', course: course.courseId ?? '', units: course.units ?? '', grade: course.finalgrade ?? '', date: formatDate(course.date ?? ''), status: course.status ?? '', tag: tagSelector(course.status ?? ''), })) ?? [] }This would provide better type checking and make the code more self-documenting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/service-portal/education/src/screens/SecondarySchoolCareer/SecondarySchoolCareer.tsx (1 hunks)
- libs/service-portal/education/src/screens/SecondarySchoolGraduationDetail/SecondarySchoolGraduationDetail.tsx (1 hunks)
- libs/service-portal/education/src/utils/tagSelector.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/service-portal/education/src/screens/SecondarySchoolCareer/SecondarySchoolCareer.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/service-portal/education/src/screens/SecondarySchoolGraduationDetail/SecondarySchoolGraduationDetail.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/service-portal/education/src/utils/tagSelector.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 not posted (1)
libs/service-portal/education/src/screens/SecondarySchoolGraduationDetail/SecondarySchoolGraduationDetail.tsx (1)
138-139: Approved: Consistent naming in footer objectThe renaming of the key from
brautarheititocoursein thefooterobject is consistent with the previous changes, maintaining overall code consistency and readability.
|
Affected services are: service-portal, Deployed services: service-portal. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/service-portal/education/src/utils/tagSelector.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/service-portal/education/src/utils/tagSelector.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."
📓 Learnings (1)
libs/service-portal/education/src/utils/tagSelector.ts (1)
Learnt from: thordurhhh PR: island-is/island.is#16078 File: libs/service-portal/education/src/utils/tagSelector.ts:3-7 Timestamp: 2024-09-23T14:05:19.772Z Learning: The `tagSelector` function maps the Icelandic strings returned from the API to matching colors, and the service currently returns Icelandic strings.
🔇 Additional comments (3)
libs/service-portal/education/src/utils/tagSelector.ts (3)
1-1: Good addition of theColorValuetypeThe introduction of the
ColorValuetype enhances type safety and aligns with TypeScript best practices. It clearly defines the possible return values of thetagSelectorfunction, which aids in code clarity and maintainability.
3-3: Improved function signature with explicit return typeThe addition of the explicit return type
ColorValueto thetagSelectorfunction signature enhances type safety and code readability. This change aligns well with TypeScript best practices and improves the overall quality of the code.
4-17: Approved refactoring to switch statementThe refactoring from an object-based mapping to a switch statement improves code readability and maintainability. The implementation correctly handles all cases, including a default case.
However, keep in mind that if the number of cases grows significantly in the future, an object lookup might be more performant. Consider benchmarking and potentially refactoring back to an object-based approach if the number of cases increases substantially.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16148 +/- ##
==========================================
+ Coverage 36.67% 36.68% +0.01%
==========================================
Files 6766 6767 +1
Lines 139340 139393 +53
Branches 39614 39621 +7
==========================================
+ Hits 51099 51134 +35
- Misses 88241 88259 +18
Flags with carried forward coverage won't be shown. Click here to find out more. see 32 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
|
thorkellmani
added
the
deprecated:automerge
* fix/init * chore: typed --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
tagSelectorfunctionality for more reliable color mapping based on input strings.