-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix(my-pages): add back empty table and footer #17577
Conversation
WalkthroughThis pull request encompasses changes across multiple components in the my-pages and health portals. The modifications primarily focus on enhancing component flexibility, improving empty state handling, and adjusting visual presentations. Key changes include adding an Changes
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
|
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 (2) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17577 +/- ##
==========================================
+ Coverage 35.56% 35.57% +0.01%
==========================================
Files 7031 7031
Lines 150510 150625 +115
Branches 42978 43120 +142
==========================================
+ Hits 53536 53592 +56
- Misses 96974 97033 +59 Flags with carried forward coverage won't be shown. Click here to find out more. see 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, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
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: 0
🧹 Nitpick comments (3)
libs/portals/my-pages/health/src/screens/MedicinePrescriptions/components/NestedInfoLines/NestedInfoLines.tsx (1)
24-27
: Consider extracting the background color mapping to improve maintainability.The styling implementation is clean and flexible. However, to improve maintainability, consider extracting the background color mapping to a constant.
+const BACKGROUND_COLOR_MAP = { + blue: 'blue100', + white: 'white', +} as const; const NestedInfoLines: React.FC<Props> = ({ label, data, width = 'full', backgroundColor, }) => { return ( <Box padding={[0, 0, 1, 3, 3]} - background={backgroundColor === 'blue' ? 'blue100' : 'white'} + background={backgroundColor ? BACKGROUND_COLOR_MAP[backgroundColor] : 'white'} >libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (1)
45-120
: LGTM! Enhanced nested table structure with a minor suggestion.The implementation properly handles empty states, comments, and styling. Consider extracting the nested table configuration into a separate constant for improved readability.
+const nestedTableLabels = { + nr: formatMessage(messages.vaccinesTableHeaderNr), + date: formatMessage(messages.vaccinesTableHeaderDate), + age: formatMessage(messages.vaccinesTableHeaderAge), + vaccine: formatMessage(messages.vaccinesTableHeaderVaccine), + location: formatMessage(messages.vaccinesTableHeaderLocation), +} children: ( <Box padding={[0, 0, 0, 3]} background={['transparent', 'transparent', 'blue100']} > <Box background="white"> <SortableTable - labels={{ - nr: formatMessage(messages.vaccinesTableHeaderNr), - date: formatMessage(messages.vaccinesTableHeaderDate), - age: formatMessage(messages.vaccinesTableHeaderAge), - vaccine: formatMessage(messages.vaccinesTableHeaderVaccine), - location: formatMessage(messages.vaccinesTableHeaderLocation), - }} + labels={nestedTableLabels} align="left" defaultSortByKey="nr"libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx (1)
251-273
: Simplify footer rendering logic.The current implementation has complex conditional logic spread across multiple locations. Consider consolidating the footer rendering logic into a separate function for better maintainability.
+const renderFooter = (footer: SortableTableProps['footer']) => { + if (!footer) return null; + if (React.isValidElement(footer)) return footer; + + return ( + <T.Row> + <T.Data + text={{ fontWeight: 'semiBold' }} + borderColor="white" + key="footer-empty" + unselectable="on" + /> + {Object.values(footer).map((valueItem) => ( + <T.Data + text={{ fontWeight: 'semiBold' }} + borderColor="white" + key={`footer-${valueItem}`} + > + {valueItem} + </T.Data> + ))} + </T.Row> + ); +}; // In the render method: -{props.footer - ? !React.isValidElement(props.footer) && ( - <T.Row> - <T.Data - text={{ fontWeight: 'semiBold' }} - borderColor="white" - key="footer-empty" - unselectable="on" - > - {/* Empty cell at index 0 */} - </T.Data> - {Object.values(props.footer).map((valueItem) => ( - <T.Data - text={{ fontWeight: 'semiBold' }} - borderColor="white" - key={`footer-${valueItem}`} - > - {valueItem} - </T.Data> - ))} - </T.Row> - ) - : null} +{renderFooter(props.footer)} -{props.footer && React.isValidElement(props.footer) - ? props.footer - : undefined}Also applies to: 277-279
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/portals/my-pages/core/src/components/MobileTable/MobileTableRow.tsx
(1 hunks)libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx
(5 hunks)libs/portals/my-pages/core/src/components/SortableTable/types.ts
(2 hunks)libs/portals/my-pages/health/src/screens/AidsAndNutrition/Aids.tsx
(1 hunks)libs/portals/my-pages/health/src/screens/MedicinePrescriptions/components/NestedInfoLines/NestedInfoLines.tsx
(1 hunks)libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/portals/my-pages/health/src/screens/AidsAndNutrition/Aids.tsx
🧰 Additional context used
📓 Path-based instructions (5)
libs/portals/my-pages/health/src/screens/MedicinePrescriptions/components/NestedInfoLines/NestedInfoLines.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/portals/my-pages/core/src/components/SortableTable/SortableTable.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/portals/my-pages/core/src/components/MobileTable/MobileTableRow.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/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.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/portals/my-pages/core/src/components/SortableTable/types.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 (9)
libs/portals/my-pages/health/src/screens/MedicinePrescriptions/components/NestedInfoLines/NestedInfoLines.tsx (2)
14-14
: LGTM! Well-defined TypeScript type.The optional
backgroundColor
prop is properly typed with a specific union type, following TypeScript best practices.
17-22
: LGTM! Clean component signature.The component signature is well-structured with proper TypeScript typing and clean props destructuring.
libs/portals/my-pages/core/src/components/SortableTable/types.ts (2)
25-29
: LGTM! Enhanced footer type flexibility.The updated footer type now supports both object and React element formats, improving component reusability.
38-38
: LGTM! Added empty state support.The new
emptyTableMessage
prop enhances UX by allowing customized empty state messages.libs/portals/my-pages/core/src/components/MobileTable/MobileTableRow.tsx (1)
48-48
: LGTM! Improved visual hierarchy.The updated divider condition now correctly handles nested tables by preventing dividers in inner tables.
libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (1)
10-10
: LGTM! Added required imports.The new imports support enhanced formatting and styling capabilities.
Also applies to: 13-13
libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx (3)
3-3
: LGTM! Added required imports.The new imports support empty state handling and footer validation.
Also applies to: 20-21
41-41
: LGTM! Updated documentation.The JSDoc is properly updated to include the new
emptyTableMessage
prop.Also applies to: 60-60
229-249
: LGTM! Added empty table handling.The implementation properly handles empty states with consistent styling.
My pages - Health - Vaccinations
What
When I added mobile tables for vaccinations, I forgot about the empty table and footer message. Sooo I added them back 😸 🤭
Also .....
ToDo -> test **Secondary School ** table with data? Not available on dev atm 😬
Checklist:
Summary by CodeRabbit
Release Notes
New Features
NestedInfoLines
component with background color optionsImprovements
SortableTable
to support more flexible footer renderingUI/UX Changes