feat(app-sys): Shared display field #17007
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17007 +/- ##
==========================================
+ Coverage 35.54% 35.56% +0.01%
==========================================
Files 6929 6929
Lines 146664 146611 -53
Branches 41693 41671 -22
==========================================
+ Hits 52135 52139 +4
+ Misses 94529 94472 -57 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 21 Total Test Services: 0 Failed, 20 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
jonnigs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx (3)
14-24: Add JSDoc documentation for better component reusability.Consider adding JSDoc documentation to describe the component's purpose, props, and usage examples. This will improve reusability across different NextJS apps.
Example addition:
+/** + * DisplayFormField - A read-only form field component that displays formatted values + * @param {Props} props - Component props + * @param {DisplayField} props.field - Display field configuration + * @param {object} props.application - Application context + * @returns {JSX.Element} Rendered form field + */ export const DisplayFormField = ({ field, application }: Props) => {
25-28: Consider safer state initialization and type handling.Two potential improvements for robustness:
- Initialize
displayValuewith a type-safe default- Validate locale type at runtime instead of using type assertion
Consider this safer implementation:
-const [displayValue, setDisplayValue] = useState(allValues[id]) +const [displayValue, setDisplayValue] = useState<string | number | null>(allValues[id] ?? null) -const { formatMessage, lang: locale } = useLocale() +const { formatMessage, lang } = useLocale() +const locale = isValidLocale(lang) ? lang : defaultLocale
36-79: Extract hardcoded values and simplify conditional rendering.A few suggestions for improvement:
- The "blue" background color should be configurable via props
- The variant type checking could be more type-safe
Consider these improvements:
+const VALID_VARIANTS = ['currency', 'textarea'] as const +type ValidVariant = typeof VALID_VARIANTS[number] +interface Props extends FieldBaseProps { + field: DisplayField + backgroundColor?: string +} -backgroundColor="blue" +backgroundColor={props.backgroundColor ?? 'blue'} -variant === 'currency' || variant === 'textarea' +VALID_VARIANTS.includes(variant as ValidVariant)libs/application/ui-fields/src/lib/index.ts (1)
34-34: LGTM! Consider organizing exports alphabetically.The new export follows the established patterns and coding guidelines, making the
DisplayFormFieldcomponent reusable across NextJS apps while enabling effective tree-shaking. However, for better maintainability, consider organizing all exports alphabetically.Here's how the new export would fit alphabetically (between
DescriptionFormFieldandDividerFormField):export { DescriptionFormField } from './DescriptionFormField/DescriptionFormField' +export { DisplayFormField } from './DisplayFormField/DisplayFormField' export { DividerFormField } from './DividerFormField/DividerFormField'libs/application/core/src/lib/fieldBuilders.ts (1)
1014-1043: LGTM! The implementation follows the established patterns.The
buildDisplayFieldfunction:
- Correctly utilizes the
extractCommonFieldsutility- Properly handles display-specific properties
- Maintains consistency with other field builders
- Aligns with the PR objective of creating a non-editable display field
Consider documenting the function with JSDoc comments to maintain consistency with other documented builders like
buildHiddenInputWithWatchedValueandbuildHiddenInput. This would help developers understand the purpose and usage of the display field.Example:
+/** + * Constructs a display field configuration object. + * This function creates a non-editable display field that automatically updates with changes to the application object. + * + * @param {Omit<DisplayField, 'type' | 'component' | 'children'>} data + * - `title`: Optional title for the display field + * - `value`: The value to be displayed + * - `suffix`: Optional suffix to be displayed after the value + * - `rightAlign`: Optional flag to align the value to the right + */ export const buildDisplayField = ( data: Omit<DisplayField, 'type' | 'component' | 'children'>, ): DisplayField => {libs/application/types/src/lib/Fields.ts (1)
801-812: Consider adding JSDoc comments for the DisplayField interface.The interface implementation looks good and aligns with the PR objective of creating a non-editable display field. However, adding JSDoc comments would improve maintainability and help other developers understand the purpose and usage of this field type.
+/** + * Represents a read-only display field that automatically updates with changes to the application object. + * This field type resembles an input field but is non-editable. + */ export interface DisplayField extends BaseField { readonly type: FieldTypes.DISPLAY component: FieldComponents.DISPLAY marginTop?: ResponsiveProp<Space> marginBottom?: ResponsiveProp<Space> titleVariant?: TitleVariants suffix?: MessageDescriptor | string rightAlign?: boolean variant?: TextFieldVariant label?: MessageDescriptor | string + /** Function that derives the display value from form answers */ value: (answers: FormValue) => string }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/types/src/lib/Fields.ts(5 hunks)libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx(1 hunks)libs/application/ui-fields/src/lib/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/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/DisplayFormField/DisplayFormField.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/ui-fields/src/lib/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."
🔇 Additional comments (7)
libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx (2)
1-12: LGTM! Well-structured imports and type definitions.
The code demonstrates good TypeScript practices with proper type definitions and modular imports from shared libraries.
1-80: Verify component's reusability across NextJS apps.
The component follows most best practices for reusability, but let's verify its integration capabilities.
libs/application/core/src/lib/fieldBuilders.ts (1)
49-49: LGTM!
The import of DisplayField type is correctly added to the existing imports from @island.is/application/types.
libs/application/types/src/lib/Fields.ts (4)
265-265: LGTM! Enum value follows naming convention.
The DISPLAY enum value in FieldTypes follows the established pattern of uppercase values.
302-302: LGTM! Component name follows convention.
The DISPLAY enum value in FieldComponents follows the established pattern of PascalCase component names with 'FormField' suffix.
850-850: LGTM! Field type union updated correctly.
The DisplayField type has been properly added to the Field union type, ensuring type safety throughout the application.
801-812: Verify the reusability of the DisplayField component.
The implementation follows TypeScript best practices and the coding guidelines for reusability across different NextJS apps. However, let's verify that this component is being used consistently across the codebase.
✅ Verification successful
Let me gather more information about the implementation and usage patterns.
The DisplayField component is well-implemented and consistently used across the application
The verification shows that the DisplayField component follows a consistent pattern and is properly integrated into the application's form field system:
- It's correctly typed in
libs/application/types/src/lib/Fields.ts - The implementation in
DisplayFormField.tsxfollows best practices:- Uses React Hook Form for form state management
- Properly handles localization
- Supports responsive styling
- Implements consistent UI components from the design system
- It's properly exported and integrated into the field builders system
- The field builder implementation (
buildDisplayField) maintains consistency with other field types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of DisplayFormField component across the codebase
echo "Searching for DisplayFormField component usage..."
rg -l "DisplayFormField"
# Search for DisplayField type usage
echo "Searching for DisplayField type usage..."
ast-grep --pattern 'DisplayField'
Length of output: 633
Script:
#!/bin/bash
# Check the implementation of DisplayFormField component
echo "DisplayFormField implementation:"
cat libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx
# Check how it's being used in fieldBuilders
echo "Field builders usage:"
cat libs/application/core/src/lib/fieldBuilders.ts
# Check for any tests
echo "Looking for related tests:"
fd "DisplayFormField" -e test.tsx -e spec.tsx
Length of output: 25174
libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx
Show resolved
Hide resolved
What
Field that looks like an input but is not editable and updates with changes to the application object
Why
This functionality is needed in a few places
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
DisplayFieldtype for enhanced form field capabilities.DisplayFormFieldcomponent for rendering display fields in forms.DisplayFormField.Bug Fixes