feat(application-system): Add phone and email fields to tablerepeater #17219
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces a new property, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
norda-gunni
changed the title
- Renamed 'rentalHousingLandlordInfoTable' to 'tableRepeaterField' in ExampleForm. - Updated ExampleSchema to include validation for the new 'tableRepeaterField' structure. - Enhanced NationalIdWithName component to accept phone and email default values. - Refactored error handling in NationalIdWithName for better validation feedback. - Cleaned up unused code in TableRepeaterFormField for improved readability.
norda-gunni
force-pushed
the
sa-add-person-field-to-tablerepeater
branch
from
bccaf47 to
e52f68a
Compare
norda-gunni
changed the title
norda-gunni
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17219 +/- ##
==========================================
- Coverage 35.74% 35.72% -0.03%
==========================================
Files 6937 6937
Lines 148284 148334 +50
Branches 42311 42404 +93
==========================================
- Hits 52998 52986 -12
- Misses 95286 95348 +62 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 30 Total Test Services: 0 Failed, 29 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
libs/application/templates/reference-template/src/forms/ExampleForm.ts (1)
27-27: Unused Import: 'applicantInformationMultiField'The import
applicantInformationMultiFieldis not used in the file. Consider removing it to keep the code clean.libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (2)
253-263: Simplify Error Handling LogicThe conditional statements for handling errors in the
nameFieldare complex and may affect readability. Consider refactoring the logic to make it more straightforward.
270-300: Conditional Rendering May Cause Layout ShiftsThe conditional rendering of
phoneandGridRowcould lead to layout shifts if only one of the fields is rendered. To maintain consistent layout, consider always rendering bothGridColumncomponents and conditionally rendering the input controllers inside them.libs/application/types/src/lib/Fields.ts (2)
Line range hint
92-107: Document New Properties in 'RepeaterItem'The properties
phoneLabel,emailLabel,phone,phoneRequired,emailRequiredhave been added toRepeaterItem. Ensure these are properly documented to maintain clarity for future developers.
630-631: Redundant 'phone' and 'email' PropertiesThe
phoneandTableRepeaterFieldmay be unnecessary if these configurations are already handled within thefieldsproperty. Verify whether these properties are required, and remove them if they serve no additional purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/application/templates/reference-template/src/forms/ExampleForm.ts(2 hunks)libs/application/templates/reference-template/src/lib/dataSchema.ts(1 hunks)libs/application/types/src/lib/Fields.ts(3 hunks)libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/reference-template/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/reference-template/src/forms/ExampleForm.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/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-components/src/components/NationalIdWithName/NationalIdWithName.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (2)
95-102: Verify 'getFieldErrorString' Functionality
Ensure that the getFieldErrorString function correctly retrieves error messages for all field types, especially when dealing with nested fields or arrays.
233-233: Possible Redundant 'error' Prop Usage
The error prop is passed to InputController for nationalIdField. Since getErrorViaPath is already used for error handling, verify that passing the error prop as getFieldErrorString(error, 'nationalId') is necessary and does not cause unexpected behavior.
libs/application/templates/reference-template/src/forms/ExampleForm.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/reference-template/src/forms/ExampleForm.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/reference-template/src/lib/dataSchema.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/reference-template/src/lib/dataSchema.ts
Outdated
Show resolved
Hide resolved
norda-gunni
force-pushed
the
sa-add-person-field-to-tablerepeater
branch
from
694164f to
11c905b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/reference-template/src/lib/dataSchema.ts (1)
18-31: Consider enhancing field validationsWhile the basic validations are in place, consider the following improvements:
- Add character set validation for the name field to prevent special characters
- Add specific phone number format validation for Icelandic numbers
tableRepeaterField: z.array( z.object({ nationalIdWithName: z.object({ - name: z.string().min(1).max(256), + name: z.string() + .min(1) + .max(256) + .regex(/^[a-zA-ZÀ-ÿ\s'-]+$/, { message: 'Name contains invalid characters' }), nationalId: z.string().refine((n) => n && kennitala.isValid(n), { params: m.dataSchemeNationalId, }), phone: z.string().refine(isValidNumber, { params: m.dataSchemePhoneNumber, }), email: z.string().email(), }), }), ),libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (2)
29-45: Enhance type safety for label propsConsider making the label props required when their corresponding feature flags are enabled.
interface NationalIdWithNameProps { // ... other props phoneLabel: phone extends true ? StaticText : never; emailLabel: email extends true ? StaticText : never; // ... other props }
266-296: Consider adding ARIA attributes for accessibilityWhile the implementation is correct, consider enhancing accessibility by adding appropriate ARIA attributes to the form fields.
<PhoneInputController id={phoneField} label={formatMessage(phoneLabel)} + aria-describedby={`${phoneField}-description`} // ... other props /> <InputController id={emailField} label={formatMessage(emailLabel)} + aria-describedby={`${emailField}-description`} // ... other props />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/application/templates/reference-template/src/lib/dataSchema.ts(1 hunks)libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/templates/reference-template/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/ui-components/src/components/NationalIdWithName/NationalIdWithName.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/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (1)
125-130: LGTM!
The default value handling for phone and email fields follows the established pattern consistently.
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (2)
57-74: Consider providing meaningful default labels.Empty string defaults for
phoneLabelandemailLabelcould result in fields without visible labels. Consider using default messages from your i18n system.- phoneLabel = '', - emailLabel = '', + phoneLabel = coreErrorMessages.phone, + emailLabel = coreErrorMessages.email,
270-300: Consider improving responsive layout.The current layout might stack fields awkwardly on smaller screens when both phone and email are enabled. Consider:
- Using a more flexible grid system
- Adding spacing between stacked fields
- {(phone || email) && ( - <GridRow> + {(phone || email) && ( + <GridRow spacing={[2, 2, 2, 3]}> {phone && ( <GridColumn span={['1/1', '1/1', '1/1', '1/2']} paddingTop={2}> <PhoneInputController
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.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 (4)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (4)
12-15: LGTM! Imports follow best practices.
The imports are properly tree-shakeable and promote reusability by using shared form field components.
29-45: LGTM! Interface changes maintain backward compatibility.
The new props are:
- Properly typed with TypeScript
- Optional, ensuring backward compatibility
- Use StaticText type for labels, supporting internationalization
91-102: Improve type safety in error handling.
The implementation uses multiple type assertions which could be unsafe. Consider using type guards instead.
129-134: LGTM! Default value handling is consistent.
The implementation follows the established pattern and properly handles fallback values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (1)
230-231: Use optional chaining for better code safetyReplace the conditional operator with optional chaining for better readability and safety.
- onNationalIdChange && - onNationalIdChange(v.target.value.replace(/\W/g, '')) + onNationalIdChange?.(v.target.value.replace(/\W/g, ''))🧰 Tools
🪛 Biome (1.9.4)
[error] 230-231: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.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."
🪛 Biome (1.9.4)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx
[error] 230-231: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (4)
13-15: LGTM: Well-structured TypeScript interface extensions
The new props for phone and email fields are properly typed and follow TypeScript best practices with appropriate optional flags.
Also applies to: 29-32, 37-38, 43-45
57-60: LGTM: Props initialization follows consistent patterns
Default values are properly set and field IDs are constructed consistently with the existing pattern.
Also applies to: 66-67, 72-74, 79-80
91-102: Improve type safety in error handling
The current implementation uses multiple type assertions which could be unsafe. Consider using type guards instead.
272-302: LGTM: Clean implementation of conditional phone and email fields
The implementation:
- Uses clean conditional rendering
- Maintains consistent error handling
- Follows responsive design patterns
- Preserves component reusability
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/reference-template/src/forms/ExampleForm.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/templates/reference-template/src/forms/ExampleForm.ts (1)
27-27: Remove unused importThe
applicantInformationMultiFieldimport appears to be unused in this file.-import { applicantInformationMultiField } from '@island.is/application/ui-forms'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/application/templates/reference-template/src/forms/ExampleForm.ts(2 hunks)libs/application/types/src/lib/Fields.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/templates/reference-template/src/forms/ExampleForm.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/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."
🔇 Additional comments (2)
libs/application/templates/reference-template/src/forms/ExampleForm.ts (1)
44-62: 🛠️ Refactor suggestion
Use message descriptors for localization
The field labels are hardcoded in Icelandic. For better internationalization support, use message descriptors for the labels.
nationalIdWithName: {
component: 'nationalIdWithName',
label: 'nationalIdWithName',
- phoneLabel: 'Símanúmer',
- emailLabel: 'Netfang',
+ phoneLabel: m.phoneNumber,
+ emailLabel: m.email,
required: true,
showPhone: true,
phoneRequired: true,
showEmail: true,
emailRequired: true,
},libs/application/types/src/lib/Fields.ts (1)
92-93: LGTM! Well-structured type definitions
The new phone and email field properties are properly typed as optional fields with clear, consistent naming that matches the codebase conventions.
Also applies to: 104-107
norda-gunni
force-pushed
the
sa-add-person-field-to-tablerepeater
branch
from
b7dcbdd to
61f7f17
Compare
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx
Outdated
Show resolved
Hide resolved
…me/NationalIdWithName.tsx Add suggestion from jonni Co-authored-by: Jónas G. Sigurðsson <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (1)
91-102: 🛠️ Refactor suggestionImprove type safety in error handling
The current implementation uses multiple type assertions which could be unsafe. Consider using type guards instead.
const getFieldErrorString = (error: unknown, id: string): string | undefined => { if (!error || typeof error !== 'object') return undefined; const errorList = error as Record<string, unknown>[]; if (!Array.isArray(errorList)) return undefined; const fieldError = errorList[id as any]; return typeof fieldError === 'string' ? fieldError : undefined; };
🧹 Nitpick comments (4)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (4)
57-60: Consider more descriptive boolean prop namesThe boolean flags
showPhoneFieldandshowEmailFieldcould be more descriptive to better indicate their purpose.Consider renaming to:
- showPhoneField?: boolean - showEmailField?: boolean + includePhoneField?: boolean + includeEmailField?: booleanAlso applies to: 72-74
129-134: Optimize default value assignmentsThe default value assignments could be more concise using nullish coalescing.
- const defaultPhone = phoneDefaultValue - ? phoneDefaultValue - : getValueViaPath<string>(application.answers, phoneField, '') - const defaultEmail = emailDefaultValue - ? emailDefaultValue - : getValueViaPath<string>(application.answers, emailField, '') + const defaultPhone = phoneDefaultValue ?? getValueViaPath<string>(application.answers, phoneField, '') + const defaultEmail = emailDefaultValue ?? getValueViaPath<string>(application.answers, emailField, '')
272-302: Consider extracting contact fields into a separate componentThe phone and email fields section could be extracted into a reusable component to improve maintainability and reusability across different forms.
Example implementation:
interface ContactFieldsProps { showPhone?: boolean; showEmail?: boolean; phoneField: string; emailField: string; phoneLabel: StaticText; emailLabel: StaticText; defaultPhone?: string; defaultEmail?: string; phoneRequired?: boolean; emailRequired?: boolean; disabled?: boolean; error?: unknown; } const ContactFields: FC<ContactFieldsProps> = ({...props}) => { return ( <GridRow> {props.showPhone && ( <GridColumn span={['1/1', '1/1', '1/1', '1/2']} paddingTop={2}> <PhoneInputController {...phoneProps} /> </GridColumn> )} {props.showEmail && ( <GridColumn span={['1/1', '1/1', '1/1', '1/2']} paddingTop={2}> <InputController {...emailProps} /> </GridColumn> )} </GridRow> ); };
230-231: Use optional chaining for callback invocationReplace the conditional operator with optional chaining for better readability.
- onNationalIdChange && - onNationalIdChange(v.target.value.replace(/\W/g, '')) + onNationalIdChange?.(v.target.value.replace(/\W/g, ''))🧰 Tools
🪛 Biome (1.9.4)
[error] 230-231: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.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."
🪛 Biome (1.9.4)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx
[error] 230-231: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
libs/application/ui-components/src/components/NationalIdWithName/NationalIdWithName.tsx (1)
4-4: LGTM! Interface and imports are well-structured
The new props and imports are properly typed and organized, following TypeScript best practices.
Also applies to: 12-15, 29-32, 37-38, 43-45
norda-gunni
added
deprecated:automerge
norda-gunni
added
the
deprecated:automerge
...
Attach a link to issue if relevant
What
Add possibility of having phone and email fields to nationalIdWithName
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