feat(new-primary-school): Update new school page to use updateOnSelect #17612
Conversation
WalkthroughThis pull request involves modifications to several files in the new primary school application template and the driving license domain. The changes primarily focus on restructuring the data schema for school-related information, simplifying the form's input handling, and updating TypeScript type definitions. The modifications streamline the data retrieval process, remove unnecessary hidden inputs, and make the code more consistent by using lowercase type definitions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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: 0
🧹 Nitpick comments (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts (1)
71-75: Consider handling edge cases in municipality selection.The
selectedValuefallback toschoolMunicipalityis good, but we should handle the case where both are undefined.- (selectedValue ? selectedValue : schoolMunicipality), + selectedValue ?? schoolMunicipality ?? '',libs/application/templates/new-primary-school/src/lib/dataSchema.ts (1)
118-121: Consider adding more validation rules.While the basic schema is correct, consider enhancing it with additional validation rules:
- Minimum length validation for both fields
- Custom validation for municipality names
newSchool: z.object({ - municipality: z.string(), - school: z.string(), + municipality: z.string().min(1, 'Municipality is required'), + school: z.string().min(1, 'School is required'), }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts(4 hunks)libs/application/templates/new-primary-school/src/lib/dataSchema.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.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/application/templates/new-primary-school/src/lib/dataSchema.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/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.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 (2)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (2)
Learnt from: veronikasif
PR: island-is/island.is#17477
File: libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts:82-115
Timestamp: 2025-01-13T14:20:07.540Z
Learning: In the new-primary-school application template, the user prefers to keep the type casting approach for extracting allergies data from application answers, rather than introducing additional type safety measures.
Learnt from: veronikasif
PR: island-is/island.is#16096
File: libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts:12-18
Timestamp: 2024-11-12T15:15:20.157Z
Learning: Be cautious when suggesting removal of imports; ensure all imports are thoroughly checked to confirm whether they are unused.
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts (1)
Learnt from: veronikasif
PR: island-is/island.is#16096
File: libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts:81-90
Timestamp: 2024-11-12T15:15:20.157Z
Learning: When checking conditions involving variables like `schoolMunicipality` and `newSchoolHiddenInput`, ensure to verify that `schoolMunicipality` has a value before comparing it to `newSchoolHiddenInput`.
🔇 Additional comments (5)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts (3)
26-26: LGTM! Consistent path updates.The ID and path updates align well with the new schema structure, making the code more maintainable.
Also applies to: 30-30, 50-50
55-56: Good UX improvement!The addition of
updateOnSelectensures that the school options are refreshed when the municipality changes, providing a better user experience.
86-89: LGTM! Simplified condition.The condition has been simplified while maintaining the same functionality. This aligns with the KISS principle.
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (2)
12-12: LGTM! Required import.The
ContactsRowimport is used in the type annotation for thecontactsvariable.
148-148: LGTM! Consistent path updates.The path updates align with the new schema structure and maintain consistency across the codebase.
Also applies to: 151-151
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17612 +/- ##
==========================================
- Coverage 35.56% 35.55% -0.01%
==========================================
Files 7033 7033
Lines 150624 150617 -7
Branches 43023 43019 -4
==========================================
- Hits 53562 53558 -4
+ Misses 97062 97059 -3
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.
|
…i-domains-air-discount-scheme,api-domains-application,api-domains-auth,api-domains-auth-admin,api-domains-document-provider,api-domains-endorsement-system update dirty files
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 (1)
|
…mains-documents,api-domains-driving-license,api-domains-driving-license-book,api-domains-education,api-domains-email-signup,api-domains-endorsement-system,api-domains-energy-funds update dirty files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.ts (1)
Line range hint
1-40: Consider removing the wildcard export for better tree-shaking.The wildcard export
export * from '@island.is/nest/audit'may affect tree-shaking capabilities. Consider explicitly exporting only the needed entities.-export * from '@island.is/nest/audit' +import { AuditService } from '@island.is/nest/audit' +export { AuditService }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.ts(1 hunks)libs/api/domains/driving-license/src/lib/graphql/qualitySignature.resolver.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/api/domains/driving-license/src/lib/graphql/qualitySignature.resolver.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.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."
🔇 Additional comments (1)
libs/api/domains/driving-license/src/lib/graphql/qualityPhoto.resolver.ts (1)
25-25: LGTM! Consider standardizing primitive types across related files.The change from
Stringtostringaligns with TypeScript best practices by using primitive types instead of wrapper objects.Run this script to check if similar changes are needed in related files:
✅ Verification successful
The change to
Promise<string>is consistent with codebase conventionsThe verification confirms that all similar return types in the codebase use the primitive
stringtype rather than theStringobject type. No additional changes are needed in other files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Promise<String> type usage in related resolver files ast-grep --pattern 'Promise<String>'Length of output: 38
Script:
#!/bin/bash # Search for Promise type annotations using ripgrep echo "=== Searching for Promise<String> ====" rg "Promise<String>" -t ts echo -e "\n=== Searching for Promise<string> ====" rg "Promise<string>" -t tsLength of output: 14880
...ew-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/newSchoolSubSection.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
...
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
Release Notes
Refactor
Bug Fixes
Style
Stringtostringin resolver methods.These changes enhance the application's data handling and improve type consistency across various components.