feat(new-primary-school): Implement language page #17530
Conversation
WalkthroughThis pull request introduces significant modifications to the language-related functionality within the New Primary School application template. The changes include a restructuring of how language information is captured, validated, and displayed. Key updates involve the introduction of new properties for language selection, the removal of outdated fields, and enhancements to the user interface components for better language management. Additionally, new utility functions and constants have been added to support these changes, resulting in a more comprehensive approach to handling a child's language background. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
✨ 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 ✅ 8 Total Test Services: 0 Failed, 7 Passed Test Services
|
- Get child language box working
- Hide child language if no language is selected
- Adding validation
- Updating overview page
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (18)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (1)
41-59: Suggestion: Simplify conditions by extracting to a shared variableThe condition functions for the
buildDescriptionFieldandbuildCustomFieldcomponents are identical. Consider extracting the condition into a shared variable or function to improve readability and maintainability.Example refactor:
const showOtherLanguages = (answers) => hasOtherLanguages(answers); // Then use in both conditions: condition: showOtherLanguages,libs/application/templates/new-primary-school/src/fields/Review/review-groups/Languages.tsx (1)
68-111: Refactor repetitiveDataValuecomponents into a dynamic listThe
DataValuecomponents forlanguage1tolanguage4are repetitive. Refactoring them into a dynamic list would reduce code duplication and enhance maintainability.Example refactor:
const languages = [language1, language2, language3, language4]; languages.map((language, index) => ( <GridColumn key={index} span={['12/12', '6/12', '6/12', '6/12']}> <DataValue label={formatMessage( newPrimarySchoolMessages.differentNeeds.languageSelectionTitle, { no: `${index + 1}` }, )} value={getLanguageByCode(language)?.name} /> </GridColumn> ));libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx (3)
24-30: Optimize state initialization.The selectedLanguages array is initialized with empty strings, which is inefficient. Consider:
- Initialize with an empty array and handle length checks in the render logic
- Or use null values instead of empty strings for better type safety
- const [selectedLanguages, setSelectedLanguages] = useState<string[]>([ - '', - '', - '', - '', - ]) + const [selectedLanguages, setSelectedLanguages] = useState<(string | null)[]>([])
57-81: Optimize useEffect dependencies and logic.The current implementation has several issues:
getApplicationAnswersin the dependency array could cause unnecessary rerenders- The logic for setting visible count is repetitive
Consider refactoring to:
- useEffect(() => { - const { language1, language2, language3, language4 } = - getApplicationAnswers(application.answers) - const selected: string[] = [] - let visibleCount = 1 - if (language1) { - selected.push(language1) - visibleCount = 1 - } - if (language2) { - selected.push(language2) - visibleCount = 2 - } - if (language3) { - selected.push(language3) - visibleCount = 3 - } - if (language4) { - selected.push(language4) - visibleCount = 4 - } - - setSelectedLanguages(selected) - setVisibleLanguagesCount(visibleCount) - }, [application, getApplicationAnswers]) + useEffect(() => { + const answers = getApplicationAnswers(application.answers) + const languages = [answers.language1, answers.language2, answers.language3, answers.language4] + const selected = languages.filter(Boolean) + setSelectedLanguages(selected) + setVisibleLanguagesCount(selected.length || 1) + }, [application.answers])
109-121: Add loading state and improve button accessibility.The "Add Language" button needs:
- Loading state while fetching language options
- Proper aria-label for accessibility
<Button icon="add" variant="text" size="small" onClick={addLanguage} disabled={visibleLanguagesCount >= 4} + loading={isLoading} + aria-label={formatMessage( + newPrimarySchoolMessages.differentNeeds.addLanguageButtonAriaLabel + )} >libs/application/templates/new-primary-school/src/lib/dataSchema.ts (5)
143-148: Consider using enums for language fields.The schema uses string types for language-related fields. Consider using
z.enum()for fields that have a predefined set of values to improve type safety and validation.- languageEnvironment: z.string(), + languageEnvironment: z.enum([ + LanguageEnvironmentOptions.ONLY_ICELANDIC, + LanguageEnvironmentOptions.ICELANDIC_AND_OTHER, + LanguageEnvironmentOptions.ONLY_FOREIGN, + ]),
151-160: Improve error message for language1 validation.The error message could be more specific about the validation context.
{ path: ['language1'], - message: 'Please select at least one language', + message: 'Please select at least one language when the environment is not Icelandic-only', },
162-174: Fix typo in error message.There's a typo in the error message for childLanguage validation.
{ path: ['childLanguage'], - message: 'Please select childrens main language', + message: "Please select child's main language", },
151-159: Consider improving the error message.While the refinement logic is correct, the error message could be more specific about the context.
- message: 'Please select at least one language', + message: 'Please select at least one language when the language environment is not Icelandic-only',
162-173: Consider improving the error message.The error message could be more descriptive about the requirement.
- message: 'Please select childrens main language', + message: "Please select the child's preferred language when multiple languages are specified",libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts (1)
223-230: Improve maintainability of language fields clearing.Consider extracting the language fields into a constant array to make the code more maintainable and reduce repetition.
+const LANGUAGE_FIELDS = [ + 'languages.language1', + 'languages.language2', + 'languages.language3', + 'languages.language4', + 'languages.childLanguage', + 'languages.interpreter', +] as const; clearLanguages: assign((context) => { const { application } = context if (hasOtherLanguages(application.answers)) { - unset(application.answers, 'languages.language1') - unset(application.answers, 'languages.language2') - unset(application.answers, 'languages.language3') - unset(application.answers, 'languages.language4') - unset(application.answers, 'languages.childLanguage') - unset(application.answers, 'languages.interpreter') + LANGUAGE_FIELDS.forEach((field) => unset(application.answers, field)) } return context }),libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (5)
359-374: Add TypeScript type for language environment options.Consider adding a type for the return value of getLanguageEnvironments to improve type safety.
+type LanguageEnvironmentOption = { + value: LanguageEnvironmentOptions; + label: MessageDescriptor; +}; -export const getLanguageEnvironments = () => { +export const getLanguageEnvironments = (): LanguageEnvironmentOption[] => {
376-384: Add JSDoc comment for hasOtherLanguages function.The function's purpose and return value should be documented.
+/** + * Checks if the selected language environment indicates languages other than Icelandic. + * @param answers - The form answers object + * @returns true if the environment is not Icelandic-only, false otherwise + */ export const hasOtherLanguages = (answers: FormValue) => {
359-374: Consider adding type annotations.The getLanguageEnvironments function could benefit from explicit type annotations.
-export const getLanguageEnvironments = () => { +export const getLanguageEnvironments = (): Array<{ value: LanguageEnvironmentOptions; label: MessageDescriptor }> => {
376-384: Consider adding JSDoc documentation.The hasOtherLanguages function would benefit from documentation explaining its purpose and return value.
+/** + * Checks if the application answers indicate the presence of languages other than Icelandic + * @param answers - The form values from the application + * @returns true if the language environment is not Icelandic-only, false otherwise + */ export const hasOtherLanguages = (answers: FormValue) => {
376-384: Enhance type safety of hasOtherLanguages function.While the function works correctly, it could benefit from stronger type safety.
Apply this diff to make the function more type-safe:
export const hasOtherLanguages = (answers: FormValue) => { const { languageEnvironment } = getApplicationAnswers(answers) - if (!languageEnvironment) { + if (!languageEnvironment || typeof languageEnvironment !== 'string') { return false } - return languageEnvironment !== LanguageEnvironmentOptions.ONLY_ICELANDIC + return Object.values(LanguageEnvironmentOptions).includes(languageEnvironment as LanguageEnvironmentOptions) && + languageEnvironment !== LanguageEnvironmentOptions.ONLY_ICELANDIC }libs/application/templates/new-primary-school/src/lib/messages.ts (2)
452-476: Maintain consistent message structure.The language environment messages follow a different pattern than other similar message groups. Consider restructuring for consistency.
- onlyIcelandicOption: { + languageEnvOnlyIcelandic: { id: 'nps.application:different.needs.language.only.icelandic.option', defaultMessage: 'Aðeins töluð íslenska', description: 'Only Icelandic spoken', }, - icelandicAndOtherOption: { + languageEnvIcelandicAndOther: { id: 'nps.application:different.needs.language.icelandic.and.other.option', defaultMessage: 'Töluð íslenska og annað/önnur tungumál', description: 'Icelandic and other language(s) spoken', }, - onlyForeignOption: { + languageEnvOnlyForeign: { id: 'nps.application:different.needs.language.only.foreign.option', defaultMessage: 'Aðeins töluð önnur tungumál en íslenska', description: 'Only other languages spoken, not Icelandic', },
484-488: Use template literal type for language number.Consider using a template literal type for the language number placeholder to ensure type safety.
+type LanguageNumber = '1' | '2' | '3' | '4'; languageSelectionTitle: { id: 'nps.application:different.needs.language.selection.title', - defaultMessage: 'Tungumál {no}', + defaultMessage: 'Tungumál {languageNo}', description: 'Language {no}', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(3 hunks)libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/Languages.tsx(3 hunks)libs/application/templates/new-primary-school/src/fields/index.ts(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/constants.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/dataSchema.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(3 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
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/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.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/lib/constants.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/Languages.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/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/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.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/LanguageSelection/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/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/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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/lib/dataSchema.ts (2)
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:79-79
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/new-primary-school/src/lib/dataSchema.ts`, for the `reasonForApplication` field, since the options come from the server, we cannot use `z.nativeEnum` or `z.enum`, and should use `z.string()` instead.
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:103-103
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/new-primary-school/src/lib/dataSchema.ts`, when comparing the `reason` field in `reasonForApplication`, which comes from the server as a string, avoid comparing it directly to `ReasonForApplicationOptions` enum values, as the enum may not include all server-provided values.
🔇 Additional comments (25)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (1)
2-15: Good practice: Utilizing reusable utility functionsThe import and use of utility functions like
getApplicationAnswers,getLanguageEnvironments, andhasOtherLanguagespromotes code reusability and maintains consistency across the application.libs/application/templates/new-primary-school/src/fields/Review/review-groups/Languages.tsx (2)
24-31: Consistent use of destructuring for improved readabilityDestructuring language-related properties from
getApplicationAnswersenhances code readability and clarity.
34-36: Handle undefined language environments gracefullyEnsure that
selectedLanguageEnvironmentis defined before accessing itslabelto prevent potential runtime errors if the environment is undefined.Consider providing a fallback value in the
formatMessagecall:-value={formatMessage(selectedLanguageEnvironment?.label || '')} +value={formatMessage(selectedLanguageEnvironment?.label || { id: 'noEnvironmentSelected', defaultMessage: 'No environment selected' })}✅ Verification successful
Empty string is not a valid formatMessage argument
The review is correct. The current code passes an empty string to
formatMessagewhenselectedLanguageEnvironmentis undefined, which can cause runtime errors asformatMessageexpects a message descriptor object. The suggested fix properly handles the undefined case by providing a valid fallback message descriptor.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find getLanguageEnvironments implementation ast-grep --pattern 'function getLanguageEnvironments() { $$$ }' # Find any type definitions or interfaces related to language environments rg -g '*.ts*' 'interface.*Language|type.*Language' -A 3 # Find where selectedLanguageEnvironment is used rg -g '*.tsx' 'selectedLanguageEnvironment' -B 2 -A 2 # Find direct usage of getLanguageEnvironments rg -g '*.ts*' 'getLanguageEnvironments\(\)'Length of output: 17762
Script:
#!/bin/bash # Find implementation of getLanguageEnvironments fd getLanguageEnvironments libs/application/templates/new-primary-school --exec cat {} # Find language environment related constants and types rg -g '*.ts*' 'type.*Environment|interface.*Environment|const.*Environment.*=' -A 3 libs/application/templates/new-primary-school # Look for formatMessage patterns with undefined handling rg -g '*.tsx' 'formatMessage\(.*\|\|' libs/application/templates/new-primary-schoolLength of output: 2898
libs/application/templates/new-primary-school/src/fields/index.ts (1)
5-5: LGTM! Export follows best practices.The named export of LanguageSelection follows the barrel pattern and enables effective tree-shaking.
libs/application/templates/new-primary-school/src/lib/constants.ts (1)
59-63: LGTM! Well-structured enum definition.The LanguageEnvironmentOptions enum follows TypeScript best practices with descriptive string literal values.
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts (1)
27-34: LGTM! Clear and descriptive property destructuring.The new language-related properties are clearly defined and follow a consistent naming pattern.
libs/application/templates/new-primary-school/src/lib/dataSchema.ts (5)
5-8: LGTM!The import statement correctly imports both
ReasonForApplicationOptionsandLanguageEnvironmentOptionsfrom the constants file.
143-148: LGTM!The schema definition for language fields is well-structured with appropriate types and optionality:
languageEnvironmentas required stringsignLanguageas required YES/NO enuminterpreteras optional stringlanguage1,language2,childLanguageas optional nullable strings
5-9: LGTM!The import statement is correctly structured and necessary for the new language environment validation.
143-148: LGTM! Well-structured schema for language preferences.The schema effectively captures language preferences with appropriate optionality and nullability, allowing for flexible data entry while maintaining data integrity.
151-160: LGTM! Clear validation logic.The refinement correctly ensures that at least one language is selected when the environment is not ONLY_ICELANDIC, with a clear error message.
libs/application/templates/new-primary-school/src/lib/NewPrimarySchoolTemplate.ts (3)
34-37: LGTM!Clean import statement that correctly imports both required functions.
223-230: LGTM!The clearLanguages action correctly unsets all language-related fields when other languages are present. The implementation aligns with the new language schema structure.
223-230: LGTM! Comprehensive language field clearing.The clearLanguages action correctly uses hasOtherLanguages to determine when to clear all language-related fields, ensuring proper state management.
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (6)
20-24: LGTM!Clean import statement that correctly imports required enums.
63-89: LGTM!The getValueViaPath calls are well-structured and properly typed for the new language-related fields.
185-192: LGTM!The return object correctly includes all the new language-related fields in a logical order.
20-25: LGTM!The imports are well-organized and necessary for the new language environment functionality.
63-89: LGTM! Well-structured language variable declarations.The language-related variables are consistently declared using getValueViaPath with appropriate type annotations and clear naming.
359-374: LGTM! Clear language environment options mapping.The function provides a well-structured mapping between language environment options and their corresponding messages.
libs/application/templates/new-primary-school/src/lib/messages.ts (5)
449-449: LGTM!The new message definitions are well-structured and provide clear, user-friendly text for:
- Language environment selection
- Language options
- Sign language usage
- Interpreter needs
Also applies to: 451-514
720-728: LGTM!The overview messages for language environment and child language preference are clear and descriptive.
449-450: LGTM! Improved clarity in language description.The updated description provides clearer context about language usage in the child's daily life.
452-476: LGTM! Comprehensive language environment messages.The messages provide clear options and descriptions for the language environment selection.
477-514: LGTM! Clear and user-friendly language selection messages.The messages effectively guide users through the language selection process with clear instructions for ranking languages.
...plate-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts
Outdated
Show resolved
Hide resolved
...emplate-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
Outdated
Show resolved
Hide resolved
...emplate-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/lib/dataSchema.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/lib/dataSchema.ts
Outdated
Show resolved
Hide resolved
- Update after coderabbit
- Fix after coderabbit review
- Fix after coderabbit review
- Fix after coderabbit review
- Fix margin
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
libs/application/templates/new-primary-school/src/lib/messages.ts (1)
449-520: Well-structured i18n messages with room for enhancement.The language-related messages are well-organized and follow i18n best practices. They provide a clear user flow from language environment selection to specific language preferences.
Consider these enhancements:
- Add placeholders for dynamic content (e.g.,
{languageCount}) to support pluralization.- Consider adding aria labels for better screen reader support.
Example enhancement for the
languagesDescriptionmessage:languagesDescription: { id: 'nps.application:different.needs.languages.description', - defaultMessage: 'Raðaðu tungumálunum eftir því hvaða tungumál er mest er notað. Það sem er mest notað er nr. 1 og svo koll af kolli.', + defaultMessage: 'Raðaðu {languageCount, plural, one {tungumálinu} other {tungumálunum}} eftir því hvaða tungumál er mest er notað. Það sem er mest notað er nr. 1 og svo koll af kolli.', description: 'Rank the languages according to which language is used the most. The most used language is number 1, and so on.', }libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (2)
55-62: Add error handling for language selection condition.While the condition for displaying the language selection is implemented, consider adding error handling in case
hasOtherLanguagesthrows an error.buildCustomField({ id: 'languages', title: '', component: 'LanguageSelection', condition: (answers) => { + try { return hasOtherLanguages(answers) + } catch (error) { + console.error('Error checking other languages:', error) + return false + } }, }),
86-106: Maintain consistent error handling across conditions.For consistency with the previous suggestion, add similar error handling to the interpreter section's condition.
condition: (answers) => { + try { return hasOtherLanguages(answers) + } catch (error) { + console.error('Error checking other languages:', error) + return false + } },libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx (2)
57-81: Optimize useEffect dependencies and initialization logic.The current implementation has two issues:
- Including
getApplicationAnswersin the dependency array may cause unnecessary re-renders- The initialization logic can be simplified using array methods
-useEffect(() => { - const { language1, language2, language3, language4 } = - getApplicationAnswers(application.answers) - const selected: string[] = [] - let visibleCount = 1 - if (language1) { - selected.push(language1) - visibleCount = 1 - } - if (language2) { - selected.push(language2) - visibleCount = 2 - } - if (language3) { - selected.push(language3) - visibleCount = 3 - } - if (language4) { - selected.push(language4) - visibleCount = 4 - } - - setSelectedLanguages(selected) - setVisibleLanguagesCount(visibleCount) -}, [application, getApplicationAnswers]) +useEffect(() => { + const answers = getApplicationAnswers(application.answers) + const languages = [answers.language1, answers.language2, answers.language3, answers.language4] + const selected = languages.filter(Boolean) + setSelectedLanguages(selected) + setVisibleLanguagesCount(selected.length || 1) +}, [application])
53-55: Improve function naming for clarity.The function name
hideChildLanguagecould be more descriptive about its purpose.-const hideChildLanguage = () => { +const shouldHideChildLanguageSelection = () => { return selectedLanguages.filter((language) => language !== '').length <= 1 }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/templates/new-primary-school/src/fields/LanguageSelection/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/differentNeedsSection/languageSubSection.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."
🔇 Additional comments (4)
libs/application/templates/new-primary-school/src/lib/messages.ts (2)
2-2: LGTM: Clean import addition.The import of
LanguageEnvironmentOptionsaligns with TypeScript best practices and supports the new language environment feature.
724-734: LGTM: Consistent overview messages.The overview messages maintain consistency with the differentNeeds section and provide clear summaries of language preferences. The implementation follows established i18n patterns.
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (1)
1-15: LGTM! Well-organized imports.The imports are properly structured and all imported utilities are used within the file.
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx (1)
83-125: LGTM! Well-implemented language selection UI.The implementation includes proper accessibility attributes and handles language selection effectively.
...ew-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts
Show resolved
Hide resolved
- Added required
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (2)
56-63: Consider adding type definitions for the LanguageSelection component props.For better reusability across different NextJS apps, consider exporting the component's prop types:
export interface LanguageSelectionProps { // Define the expected props here value: string[] onChange: (value: string[]) => void }
83-84: Consider destructuring with default value for safer access.To handle potential undefined values more safely:
-const { languageEnvironment } = getApplicationAnswers(answers) +const { languageEnvironment = null } = getApplicationAnswers(answers)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.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 (3)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (3)
1-15: LGTM! Well-structured imports following best practices.The imports are properly organized and support effective tree-shaking by using named imports.
32-43: Required validation has been properly implemented.The language environment field now includes required validation as previously suggested.
51-53: Verify edge cases in conditional logic.Multiple fields use
hasOtherLanguages(answers)for conditional rendering. Let's verify the implementation to ensure all edge cases are handled correctly.Also applies to: 60-62, 105-107
✅ Verification successful
The conditional logic implementation is safe and handles all edge cases correctly
The
hasOtherLanguagesfunction has a robust implementation that:
- Safely handles undefined values
- Uses a well-defined enum with exhaustive options (ONLY_ICELANDIC, ICELANDIC_AND_OTHER, ONLY_FOREIGN)
- Has simple boolean logic that cannot fail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the hasOtherLanguages implementation and its test coverage echo "Searching for hasOtherLanguages implementation..." rg -A 10 "export const hasOtherLanguages" echo "Searching for test cases..." rg -A 10 "describe.*hasOtherLanguages"Length of output: 1223
Script:
#!/bin/bash # Search for enum and related implementations echo "Searching for LanguageEnvironmentOptions..." rg "enum LanguageEnvironmentOptions" -A 10 echo "Searching for getApplicationAnswers..." rg "export const getApplicationAnswers" -A 5 echo "Searching for tests with different pattern..." rg -g "*.test.ts" -g "*.spec.ts" "languageEnvironment.*ONLY_ICELANDIC"Length of output: 8286
Script:
#!/bin/bash # Search for languageEnvironment usage and getValueViaPath echo "Searching for languageEnvironment path..." rg "languageEnvironment.*getValueViaPath" -A 5 echo "Searching for getValueViaPath implementation..." rg "export const getValueViaPath" -A 5 echo "Searching for hasOtherLanguages tests with broader pattern..." rg -g "*.test.ts" -g "*.spec.ts" "hasOtherLanguages|languageEnvironment"Length of output: 1615
- Added error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx (3)
26-32: Optimize state initialization.The initial state for selected languages could be more concise using Array.fill().
- const [selectedLanguages, setSelectedLanguages] = useState<string[]>([ - '', - '', - '', - '', - ]) + const [selectedLanguages, setSelectedLanguages] = useState<string[]>( + Array(4).fill('') + )
87-115: Enhance accessibility for language selection.Consider adding ARIA labels and descriptions to improve accessibility.
<SelectController key={languageIdsArray[index]} label={formatMessage( newPrimarySchoolMessages.differentNeeds.languageSelectionTitle, { no: `${index + 1}` }, )} + aria-label={formatMessage( + newPrimarySchoolMessages.differentNeeds.languageSelectionAriaLabel, + { no: `${index + 1}` }, + )} + aria-description={formatMessage( + newPrimarySchoolMessages.differentNeeds.languageSelectionDescription, + )} placeholder={formatMessage( newPrimarySchoolMessages.differentNeeds.languageSelectionPlaceholder, )} id={languageIdsArray[index]} name={languageIdsArray[index]} backgroundColor="blue" options={allLanguageOptions} />
129-152: Optimize performance with memoization.Consider memoizing the filtered language options to prevent unnecessary recalculations.
+ const filteredLanguageOptions = useMemo( + () => allLanguageOptions.filter((language) => + selectedLanguages.includes(language.value) + ), + [allLanguageOptions, selectedLanguages] + ) return ( // ... <SelectController options={filteredLanguageOptions} /> )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/dataSchema.ts(2 hunks)libs/application/templates/new-primary-school/src/lib/messages.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/differentNeedsSection/languageSubSection.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/LanguageSelection/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/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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/lib/dataSchema.ts (2)
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:79-79
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/new-primary-school/src/lib/dataSchema.ts`, for the `reasonForApplication` field, since the options come from the server, we cannot use `z.nativeEnum` or `z.enum`, and should use `z.string()` instead.
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:103-103
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/new-primary-school/src/lib/dataSchema.ts`, when comparing the `reason` field in `reasonForApplication`, which comes from the server as a string, avoid comparing it directly to `ReasonForApplicationOptions` enum values, as the enum may not include all server-provided values.
🔇 Additional comments (7)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts (3)
1-15: Well-structured imports and dependencies!The imports are properly organized, and the new utility functions support the enhanced language selection functionality.
43-62: Clean implementation of language selection!The conditional rendering and use of a custom component for language selection provides a good user experience and maintainable code.
32-42: 🛠️ Refactor suggestionAdd required validation for language environment selection.
The language environment field appears to be a crucial part of the form, but lacks the
requiredflag. Consider adding validation to ensure users provide this information.buildSelectField({ id: 'languages.languageEnvironment', dataTestId: 'languages-language-environment', title: newPrimarySchoolMessages.differentNeeds.languageEnvironmentTitle, placeholder: newPrimarySchoolMessages.differentNeeds.languageEnvironmentPlaceholder, + required: true, options: () => { return getLanguageEnvironments() }, }),Likely invalid or redundant comment.
libs/application/templates/new-primary-school/src/fields/LanguageSelection/index.tsx (1)
1-11: Well-structured React component with proper TypeScript typing!The component follows React best practices with proper prop typing and clear organization.
Also applies to: 20-24
libs/application/templates/new-primary-school/src/lib/dataSchema.ts (2)
143-148: Well-structured schema definition for language fields!The schema properly defines optional and nullable fields, providing flexibility while maintaining type safety.
175-183: 🛠️ Refactor suggestionAdd missing error message for interpreter validation.
The refinement rule for interpreter is missing an error message.
{ path: ['interpreter'], + params: errorMessages.interpreterRequired, },Likely invalid or redundant comment.
libs/application/templates/new-primary-school/src/lib/messages.ts (1)
451-520: Well-organized and comprehensive message definitions!The messages are properly structured and provide clear, user-friendly text for all language-related fields and options.
...ew-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/languageSubSection.ts
Show resolved
Hide resolved
TS-949
What
Update language page to latest design
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
LanguageSelectioncomponent for dynamic language inputImprovements
Changes