-
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(social-insurance): UI changes #16942
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Possibly related PRs
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 #16942 +/- ##
==========================================
+ Coverage 35.62% 35.91% +0.28%
==========================================
Files 6915 6913 -2
Lines 146037 145323 -714
Branches 41457 41350 -107
==========================================
+ Hits 52021 52188 +167
+ Misses 94016 93135 -881
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 93 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 3 Passed Test Services
|
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: 2
🧹 Outside diff range and nitpick comments (4)
libs/clients/social-insurance-administration/src/lib/dto/incomePlan.dto.ts (2)
Line range hint
15-24
: Remove commented out code.Instead of keeping the old implementation in comments, rely on version control history to track these changes. This helps maintain cleaner and more maintainable code.
-/* -export const mapIncomePlanDto = ( - data: TrWebCommonsExternalPortalsApiModelsIncomePlanIncomePlanDto, -): IncomePlanDto | undefined => ({ - year: data.year ?? undefined, - registrationDate: data.registrationDate ?? undefined, - status: data.status as IncomePlanStatus, - origin: data.origin ?? undefined, - incomeTypeLines: data.incomeTypeLines ?? undefined, -}) */
26-56
: Verify alignment with PR objectives.The PR description mentions UI changes, but this modification to the DTO mapping layer seems to be a significant data layer change. This could have unintended consequences on the UI behavior and should be clarified.
Please clarify:
- Is this change temporary for testing purposes?
- How does this data layer change relate to the UI changes mentioned in the PR?
- What's the impact on other components that might be consuming this DTO?
libs/portals/my-pages/social-insurance-maintenance/src/screens/IncomePlan/IncomePlan.tsx (2)
Line range hint
1-16
: Remove unused FootNote importThe FootNote component is imported but not used in the code. Remove it to improve tree-shaking effectiveness.
- import { - ActionCard, - CardLoader, - FootNote, - IntroHeader, - IntroWrapper, - LinkButton, - m as coreMessages, - formatDate, - } from '@island.is/portals/my-pages/core' + import { + ActionCard, + CardLoader, + IntroHeader, + IntroWrapper, + LinkButton, + m as coreMessages, + formatDate, + } from '@island.is/portals/my-pages/core'
Line range hint
1-183
: Consider extracting URL generation logicTo improve modularity and reusability across the NextJS apps, consider extracting the URL generation logic into a shared utility function. This would centralize the logic and make it easier to maintain across different parts of the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/clients/social-insurance-administration/src/lib/dto/incomePlan.dto.ts
(2 hunks)libs/portals/my-pages/social-insurance-maintenance/src/screens/IncomePlan/IncomePlan.tsx
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/clients/social-insurance-administration/src/lib/dto/incomePlan.dto.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/portals/my-pages/social-insurance-maintenance/src/screens/IncomePlan/IncomePlan.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."
🔇 Additional comments (2)
libs/clients/social-insurance-administration/src/lib/dto/incomePlan.dto.ts (1)
26-28
:
Unused parameter 'data' should be removed if not needed.
The function accepts a parameter but never uses it. If this is intentional, remove the parameter to avoid confusion.
-export const mapIncomePlanDto = (
- data: TrWebCommonsExternalPortalsApiModelsIncomePlanIncomePlanDto,
-): IncomePlanDto | undefined => ({
+export const mapIncomePlanDto = (): IncomePlanDto | undefined => ({
Likely invalid or redundant comment.
libs/portals/my-pages/social-insurance-maintenance/src/screens/IncomePlan/IncomePlan.tsx (1)
79-86
: Well-structured component composition
The replacement of Box with IntroWrapper improves reusability and provides better semantic structure with properly typed props. The service provider information is cleanly integrated.
libs/clients/social-insurance-administration/src/lib/dto/incomePlan.dto.ts
Outdated
Show resolved
Hide resolved
libs/portals/my-pages/social-insurance-maintenance/src/screens/IncomePlan/IncomePlan.tsx
Show resolved
Hide resolved
Affected services are: service-portal, Deployed services: service-portal. |
…splay' into fix/payment-plan-display
* feat: ini * fix: switch link * chore: remove mocks --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
IncomePlan
component to dynamically adjust URLs based on the income plan's status.Bug Fixes
FootNote
component to improve footer display logic.Refactor
Box
component withIntroWrapper
for better structure.