feat(new-primary-school): Child info page #17514
Conversation
WalkthroughThis pull request introduces several enhancements to the New Primary School application template, focusing on improving form field functionality and user experience. The changes include adding a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
|
![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 (3)
libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (1)
52-52: LGTM! Consider adding type safety for the tooltip property.The implementation correctly handles the tooltip property and follows the component reusability guidelines. However, consider adding explicit type definition for the tooltip property to enhance type safety.
interface Props extends FieldBaseProps { field: TextField & { + tooltip?: string } }Also applies to: 74-79
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts (1)
19-19: Enhance type safety for the conditional DTO fields.The conditional logic for including preferred name and pronouns is correct. However, consider adding type definitions for these optional fields to improve type safety and documentation.
interface FormDto { type: FormDtoTypeEnum.Registration user: { name: string nationalId: string + preferredName?: string + pronouns?: string[] domicile: { address: string postCode: string } } // ... rest of the interface }Also applies to: 99-104
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (1)
89-96: Extract duplicate condition logic to improve maintainability.The condition for checking
usePronounAndPreferredNameis duplicated. Consider extracting it to a helper function.+const shouldShowPreferredNameAndPronouns = (answers: Application['answers']) => { + const { usePronounAndPreferredName } = getApplicationAnswers(answers) + return usePronounAndPreferredName?.includes(YES) +} buildTextField({ id: 'childInfo.preferredName', tooltip: newPrimarySchoolMessages.childrenNParents.preferredNameTooltip, - condition: (answers) => { - const { usePronounAndPreferredName } = getApplicationAnswers(answers) - return usePronounAndPreferredName?.includes(YES) - }, + condition: shouldShowPreferredNameAndPronouns, // ... rest of the config }), buildCustomField({ id: 'childInfo.pronouns', - condition: (answers) => { - const { usePronounAndPreferredName } = getApplicationAnswers(answers) - return usePronounAndPreferredName?.includes(YES) - }, + condition: shouldShowPreferredNameAndPronouns, // ... rest of the config }),Also applies to: 107-112
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(2 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(4 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(2 hunks)libs/application/types/src/lib/Fields.ts(1 hunks)libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx(2 hunks)libs/shared/form-fields/src/lib/InputController/InputController.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
libs/application/types/src/lib/Fields.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/ui-fields/src/lib/TextFormField/TextFormField.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/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/core/src/lib/fieldBuilders.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/shared/form-fields/src/lib/InputController/InputController.tsx (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
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/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.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/messages.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/childrenNParentsSection/childInfoSubSection.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/application/templates/new-primary-school/src/lib/messages.ts (1)
Learnt from: veronikasif
PR: island-is/island.is#16096
File: libs/application/templates/new-primary-school/src/lib/messages.ts:204-211
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `libs/application/templates/new-primary-school/src/lib/messages.ts`, the translation of `'Fornafn'` as `'Pronoun'` is acceptable and does not need correction.
🔇 Additional comments (10)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (2)
75-86: LGTM! Well-structured checkbox implementation.The checkbox field is correctly implemented with clear labeling and proper value handling.
130-132: LGTM! Clear and helpful description added.The description for the different place of residence field improves user understanding.
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (1)
27-30: LGTM! Clean integration with existing utilities.The implementation follows the established patterns for answer retrieval and maintains consistency with the existing codebase.
Also applies to: 154-154
libs/shared/form-fields/src/lib/InputController/InputController.tsx (3)
53-53: LGTM: Well-typed tooltip property additionThe optional tooltip property is properly typed as string, following TypeScript best practices for component props.
102-102: LGTM: Clean props destructuringThe tooltip prop is correctly destructured along with other props, maintaining consistent code style.
242-242: LGTM: Proper tooltip propagationThe tooltip is correctly passed to the underlying Input component, ensuring proper propagation of the property.
libs/application/types/src/lib/Fields.ts (1)
411-411: LGTM: Well-typed TextField tooltip propertyThe tooltip property is correctly typed as FormText, maintaining type safety and consistency with the application's type system.
libs/application/core/src/lib/fieldBuilders.ts (1)
299-299: LGTM: Proper tooltip handling in field builderThe tooltip property is correctly extracted and included in the field configuration, maintaining consistency with the builder pattern.
Also applies to: 317-317
libs/application/templates/new-primary-school/src/lib/messages.ts (2)
226-239: LGTM: Well-structured messages for preferred name and pronounsThe new messages are well-organized with clear IDs, default messages, and descriptive comments. The tooltip message provides comprehensive guidance for guardians regarding name and gender changes.
242-255: LGTM: Consistent terminology update for residenceThe terminology change from 'Dvalarstaður' to 'Aðsetur' is applied consistently across related messages, with appropriate descriptions clarifying the meaning of temporary residence.
Preferred name and pronouns behind checkmark and change 'Dvalarstaður' to 'Aðsetur'
helgifr
force-pushed
the
feat/mms-child-info-page
branch
from
49df9cb to
e061d7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (2)
75-86: Consider accessibility improvements for the checkbox field.While the implementation is functionally correct, consider moving the label text to the
titleproperty for better accessibility. Screen readers typically announce form controls using their title.buildCheckboxField({ id: 'childInfo.usePronounAndPreferredName', - title: '', + title: newPrimarySchoolMessages.childrenNParents.usePronounAndPreferredName, options: [ { value: YES, - label: - newPrimarySchoolMessages.childrenNParents - .usePronounAndPreferredName, + label: '', }, ], }),
89-96: Consider extracting the condition logic for reusability.The condition logic could be reused across multiple fields. Consider extracting it to a utility function:
const shouldShowPreferredFields = (answers: unknown) => { const { usePronounAndPreferredName } = getApplicationAnswers(answers) return usePronounAndPreferredName?.includes(YES) }Then use it in both the preferred name and pronouns fields:
condition: (answers) => { - const { usePronounAndPreferredName } = getApplicationAnswers(answers) - return usePronounAndPreferredName?.includes(YES) + return shouldShowPreferredFields(answers) },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(2 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(4 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(2 hunks)libs/application/types/src/lib/Fields.ts(1 hunks)libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx(2 hunks)libs/shared/form-fields/src/lib/InputController/InputController.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx
- libs/shared/form-fields/src/lib/InputController/InputController.tsx
- libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
- libs/application/types/src/lib/Fields.ts
- libs/application/templates/new-primary-school/src/lib/messages.ts
- libs/application/core/src/lib/fieldBuilders.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.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 (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (2)
3-3: LGTM!The import follows effective tree-shaking practices by importing only the required function.
130-132: LGTM!The addition of a description improves user understanding and follows the form builder pattern correctly.
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (2)
71-82: Consider adding a title for better accessibility.The checkbox field has an empty title which might affect screen readers. Consider adding a descriptive title that matches the functionality.
buildCheckboxField({ id: 'childInfo.usePronounAndPreferredName', - title: '', + title: newPrimarySchoolMessages.childrenNParents.usePronounAndPreferredNameTitle, options: [ { value: YES, label: newPrimarySchoolMessages.childrenNParents .usePronounAndPreferredName, }, ], }),
87-92: Extract condition logic to a reusable function.The condition logic for checking preferred name visibility could be extracted to a reusable function, as it's also used in the pronouns field.
+const shouldShowPreferredFields = (answers) => { + const { usePronounAndPreferredName } = getApplicationAnswers(answers) + return usePronounAndPreferredName?.includes(YES) +} condition: (answers) => { - const { usePronounAndPreferredName } = getApplicationAnswers(answers) - - return usePronounAndPreferredName?.includes(YES) + return shouldShowPreferredFields(answers) },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(2 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(4 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(2 hunks)libs/application/types/src/lib/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
- libs/application/types/src/lib/Fields.ts
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
- libs/application/core/src/lib/fieldBuilders.ts
- libs/application/templates/new-primary-school/src/lib/messages.ts
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.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/fields/Review/review-groups/Child.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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx (1)
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx:48-50
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the React component `Child` in `libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx`, when rendering the `SkeletonLoader`, display it only if `childInfo.pronouns` is not empty or undefined, to ensure the main content renders when pronoun data is absent.
🔇 Additional comments (3)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx (1)
105-105: LGTM! Grid layout changes improve visual consistency.The GridColumn span adjustment to '5/12' for larger screens aligns with other similar fields in the component, creating a more balanced and consistent layout.
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (2)
103-108: Extract condition logic to a reusable function.The condition logic for checking pronouns visibility should use the same extracted function suggested for the preferred name field.
126-128: LGTM! Description addition improves user guidance.The addition of a description to the radio field enhances user understanding of the different place of residence option.
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Show resolved
Hide resolved
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Show resolved
Hide resolved
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Show resolved
Hide resolved
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
Outdated
Show resolved
Hide resolved
...primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
libs/application/templates/new-primary-school/src/fields/Gender/index.tsx (3)
12-15: Add JSDoc documentation for the component props.Component lacks documentation for its props and purpose. Consider adding JSDoc comments to improve maintainability.
23-25: Consider cleanup in useEffect.The effect sets form values but doesn't handle component unmount, which could lead to memory leaks.
useEffect(() => { setValue(id, genderText) + return () => { + // Cleanup by removing the field value on unmount if needed + // setValue(id, undefined) + } }, [id, genderText, setValue])
27-42: Consider memoizing the field object.The field object is recreated on every render. Consider using useMemo to optimize performance:
+ import React, { FC, useEffect, useMemo } from 'react' // ... + const fieldConfig = useMemo(() => ({ + id, + disabled, + defaultValue: genderText, + title, + width, + type: FieldTypes.TEXT, + component: FieldComponents.TEXT, + children: undefined, + }), [id, disabled, genderText, title, width]) return ( <TextFormField application={application} showFieldName - field={{ - id, - disabled, - defaultValue: genderText, - title, - width, - type: FieldTypes.TEXT, - component: FieldComponents.TEXT, - children: undefined, - }} + field={fieldConfig} /> )libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (1)
96-100: Extract duplicated condition logic.The same condition logic is used in both preferred name and pronouns fields. Consider extracting it to a utility function:
+const shouldShowPreferredFields = (answers) => { + const { childInfo } = getApplicationAnswers(answers) + return !!childInfo.usePronounAndPreferredName?.includes(YES) +} condition: (answers) => { - const { childInfo } = getApplicationAnswers(answers) - return !!childInfo.usePronounAndPreferredName?.includes(YES) + return shouldShowPreferredFields(answers) },libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (1)
338-357: Simplify the gender code mapping.The switch statement can be simplified by removing redundant cases that fall through to default.
export const formatGender = (genderCode?: string): MessageDescriptor => { switch (genderCode) { case '1': case '3': return newPrimarySchoolMessages.shared.male case '2': case '4': return newPrimarySchoolMessages.shared.female - case '7': - case '8': default: return newPrimarySchoolMessages.shared.otherGender } }🧰 Tools
🪛 Biome (1.9.4)
[error] 346-346: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 347-347: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(1 hunks)libs/application/templates/new-primary-school/src/fields/Gender/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx(5 hunks)libs/application/templates/new-primary-school/src/fields/index.ts(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(6 hunks)libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(2 hunks)libs/application/templates/new-primary-school/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
🧰 Additional context used
📓 Path-based instructions (7)
libs/application/templates/new-primary-school/src/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."
libs/application/templates/new-primary-school/src/fields/index.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/NewPrimarySchoolTemplate.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/messages.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/fields/Gender/index.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/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.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/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."
🪛 Biome (1.9.4)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
[error] 346-346: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 347-347: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (7)
libs/application/templates/new-primary-school/src/fields/index.ts (1)
3-3: LGTM! Export follows consistent pattern.The new Gender export aligns with the existing barrel pattern and follows the libs directory guidelines for TypeScript usage.
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (4)
71-76: Well-implemented gender field!The gender field is correctly implemented as a read-only field using a custom component, maintaining consistency with the form's message system.
77-89: Clean checkbox implementation!The checkbox is well-implemented with zero spacing as requested, and the label text is appropriately sourced from messages.
109-113: Apply the same condition refactor here.Use the suggested utility function here as well to maintain DRY principles.
131-133: Good improvements to place of residence handling!The changes correctly implement:
- Added description for better user guidance
- Updated conditions to use the proper path for differentPlaceOfResidence
Also applies to: 155-157, 167-169
libs/application/templates/new-primary-school/src/lib/messages.ts (2)
108-127: Well-structured gender messages!The gender-related messages are consistently defined with clear IDs, default messages, and descriptions.
238-259: Good message updates for preferred name and pronouns!The changes:
- Correctly update pronoun terminology to "Persónufornafn"
- Add clear guidance in the tooltip about name changes
- Provide inclusive language in the checkbox label
libs/application/templates/new-primary-school/src/fields/Gender/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (1)
77-89: Consider adding a descriptive title for the checkbox field.While the implementation is correct, the empty title (
title: '') might affect accessibility. Consider adding a descriptive title or document why it's intentionally left empty.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(6 hunks)libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.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 (5)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (5)
2-2: LGTM: Import added for new checkbox field.The addition of
buildCheckboxFieldimport aligns with the new checkbox implementation.
71-76: LGTM: Gender field added as requested.The gender field has been correctly implemented as a disabled custom field using the shared message for the title.
94-100: Extract duplicated condition logic to a utility function.The condition logic for checking
usePronounAndPreferredNameis duplicated across fields. Consider extracting it to a shared utility function for better maintainability.+const shouldShowPreferredFields = (answers) => { + const { childInfo } = getApplicationAnswers(answers) + return !!childInfo?.usePronounAndPreferredName?.includes(YES) +} + condition: (answers) => { - const { childInfo } = getApplicationAnswers(answers) - return !!childInfo?.usePronounAndPreferredName?.includes(YES) + return shouldShowPreferredFields(answers) },
131-133: LGTM: Place of residence field updates.The changes correctly:
- Add a descriptive text for the different place of residence option
- Update conditions to use the proper
childInfoprefixAlso applies to: 155-157, 167-169
109-113: Verify FriggOptionsAsyncSelectField component functionality.Please ensure that:
- The async options loading works correctly for pronouns
- The multi-select functionality is properly tested
- The component handles loading and error states appropriately
Run this script to verify the component's usage:
✅ Verification successful
FriggOptionsAsyncSelectField component implementation verified successfully
The component is properly implemented with:
- Async options loading through GraphQL
- Multi-select support
- Proper error and loading state handling
- Robust language fallback mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of FriggOptionsAsyncSelectField to verify consistent implementation ast-grep --pattern 'component: "FriggOptionsAsyncSelectField"'Length of output: 64
Script:
#!/bin/bash # Search for component name variations and imports rg -i "friggoptionsasyncselect" -A 2 -B 2 # Search for potential imports of the component rg "import.*FriggOptionsAsyncSelect.*from" # Search for field type definitions that might reference this component ast-grep --pattern 'field: { $$$ component: $_, $$$ }'Length of output: 6829
Script:
#!/bin/bash # Get the component implementation details cat libs/application/templates/new-primary-school/src/fields/FriggOptionsAsyncSelectField/index.tsx # Look for test files rg -g "*test*" -i "friggoptionsasyncselect"Length of output: 2791
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/fields/Gender/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx (1)
Line range hint
97-109: Consider extracting the condition to a utility function.The condition
childInfo.usePronounAndPreferredName?.includes(YES) && childInfo.preferredName?.trim().length > 0is used multiple times. Consider extracting it to a utility function for better maintainability.+const shouldShowPreferredName = (childInfo) => + childInfo.usePronounAndPreferredName?.includes(YES) && + childInfo.preferredName?.trim().length > 0; {shouldShowPreferredName(childInfo) && ( <GridColumn span={['12/12', '12/12', '12/12', '5/12']}> <DataValuelibs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx (1)
27-49: Consider memoizing the computed values and formatted message.The
computedValuesandformattedMessagecalculations could benefit from memoization to prevent unnecessary recalculations on re-renders.+import { useMemo } from 'react' const DynamicDisabledText: FC<DynamicDisabledTextProps> = ({ field, application, }) => { const { lang, formatMessage } = useLocale() const { setValue } = useFormContext() const { title, id, width, props: { descriptor, values }, } = field - const computedValues = values?.(application, lang) + const computedValues = useMemo( + () => values?.(application, lang), + [values, application, lang] + ) - const message = - typeof descriptor === 'function' ? descriptor(application) : descriptor - const formattedMessage = formatMessage(message, computedValues) + const formattedMessage = useMemo(() => { + const message = + typeof descriptor === 'function' ? descriptor(application) : descriptor + return formatMessage(message, computedValues) + }, [descriptor, application, formatMessage, computedValues])libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts (1)
10-14: Consider adding type definitions for utility functions.The imported utility functions (
formatGrade,getApplicationExternalData,getCurrentSchoolName) should have their TypeScript interfaces documented in the utils file for better type safety and developer experience.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/fields/Grade/index.tsx(0 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx(5 hunks)libs/application/templates/new-primary-school/src/fields/index.ts(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(7 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts(2 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/new-primary-school/src/fields/Grade/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/templates/new-primary-school/src/fields/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.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/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.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/fields/DynamicDisabledText/index.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/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.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/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx (2)
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx:48-50
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the React component `Child` in `libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx`, when rendering the `SkeletonLoader`, display it only if `childInfo.pronouns` is not empty or undefined, to ensure the main content renders when pronoun data is absent.
Learnt from: helgifr
PR: island-is/island.is#17514
File: libs/application/templates/new-primary-school/src/fields/Gender/index.tsx:19-19
Timestamp: 2025-01-20T16:26:39.233Z
Learning: In the new-primary-school application, the `formatGender` function in `newPrimarySchoolUtils.ts` always returns a valid MessageDescriptor by having an exhaustive switch statement with a default case that returns otherGender, making error handling unnecessary for the `getGenderMessage` function.
🔇 Additional comments (10)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Child.tsx (3)
18-18: LGTM! Clean implementation of gender message handling.The changes effectively integrate the gender message functionality while maintaining clean code practices.
Also applies to: 29-29, 37-38
90-97: LGTM! Gender display implementation meets requirements.The gender display has been properly implemented as requested in the past review comments, maintaining consistency with the existing UI patterns.
Line range hint
109-141: LGTM! Comprehensive implementation of pronouns and residence display.The implementation properly handles:
- Conditional rendering of pronouns
- Error states for the data provider
- Consistent address formatting
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts (4)
72-82: LGTM! Clean implementation of the gender field.The gender field is properly implemented using DynamicDisabledText with appropriate descriptor handling.
83-95: LGTM! Checkbox implementation follows UI requirements.The checkbox field is properly implemented with zero spacing as requested in the past review comments.
100-106: LGTM! Well-implemented preferred name field enhancements.The additions of tooltip and conditional visibility improve the user experience while maintaining clean code practices.
Line range hint
137-175: LGTM! Place of residence implementation is complete and well-structured.The changes properly implement:
- Descriptive text for better user guidance
- Correct conditional logic for dependent fields
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx (3)
10-25: Well-structured TypeScript interface definition.The
DynamicDisabledTextPropsinterface is well-defined with proper TypeScript types, making the component type-safe and reusable. The use of discriminated unions for thedescriptorproperty provides good flexibility.
46-48: Consider the necessity of the useEffect.The
useEffecthook updates the form value on everyformattedMessagechange. Consider if this is the intended behavior, as it might cause unnecessary form updates.Please confirm if the form value should be updated every time the formatted message changes, or if it should only be set once on component mount.
50-65: LGTM: Clean TextFormField implementation.The TextFormField implementation is clean and properly typed, with all necessary props passed correctly.
...rimary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts (1)
48-58:⚠️ Potential issueAdd error handling for external data access.
The code lacks error handling for missing or malformed external data, which could lead to runtime errors.
value: (application: Application, lang: Locale) => { + if (!application.externalData) { + return { + ...newPrimarySchoolMessages.primarySchool.currentGrade, + values: { grade: '' }, + } + } const { childGradeLevel } = getApplicationExternalData( application.externalData, ) + if (childGradeLevel === undefined) { + return { + ...newPrimarySchoolMessages.primarySchool.currentGrade, + values: { grade: '' }, + } + } return { ...newPrimarySchoolMessages.primarySchool.currentGrade, values: { grade: formatGrade(childGradeLevel, lang), }, } },
🧹 Nitpick comments (3)
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx (2)
43-45: Consider adding cleanup in useEffect.The effect sets form values but doesn't clean up when the component unmounts, which could lead to stale form data.
useEffect(() => { setValue(id, defaultValue) + return () => { + setValue(id, undefined) + } }, [id, defaultValue, setValue])
48-61: Consider extracting field configuration to a constant.The field configuration object could be memoized outside the component to optimize renders.
+const getFieldConfig = (id: string, defaultValue: string, title: string, width: string) => ({ + id, + disabled: true, + defaultValue, + title, + width, + type: FieldTypes.TEXT, + component: FieldComponents.TEXT, + children: undefined, +}) return ( <TextFormField application={application} showFieldName - field={{ - id, - disabled: true, - defaultValue, - title, - width, - type: FieldTypes.TEXT, - component: FieldComponents.TEXT, - children: undefined, - }} + field={getFieldConfig(id, defaultValue, title, width)} /> )libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts (1)
52-57: Consider memoizing the messages object.The spread of
newPrimarySchoolMessages.primarySchool.currentGradeon every render could affect performance. Consider extracting it to a constant.+const currentGradeMessage = newPrimarySchoolMessages.primarySchool.currentGrade return { - ...newPrimarySchoolMessages.primarySchool.currentGrade, + ...currentGradeMessage, values: { grade: formatGrade(childGradeLevel, lang), }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts(7 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/childrenNParentsSection/childInfoSubSection.ts
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/currentSchoolSubSection.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/fields/DynamicDisabledText/index.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 (1)
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx (1)
14-21: LGTM! Props interface is well-defined with proper TypeScript types.The interface follows best practices for type safety and reusability across NextJS apps.
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/fields/DynamicDisabledText/index.tsx
Show resolved
Hide resolved
Preferred name and pronouns behind checkmark and change 'Dvalarstaður' to 'Aðsetur'
Also added tooltip to TextField
...
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
Bug Fixes
Documentation
Refactor