Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(application-system): Improving form field styling consistency #17217

Merged
merged 16 commits into from
Dec 17, 2024

Conversation

HjorturJ
Copy link
Member

@HjorturJ HjorturJ commented Dec 12, 2024

Improving form field styling consistency by adding fields to the base field object

Task link

What

Make sure its possible to adjust field styling in a similar way for each field

Screenshots / Gifs

No changes to default look, now possible to add marginTop and marginBottom to all fields who extend base field.

Before:
main

After:
styles

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

  • New Features

    • Added marginTop and marginBottom properties to various form fields for enhanced layout flexibility.
    • Updated components to utilize the Box component for improved styling and spacing control.
  • Bug Fixes

    • Improved error handling in the FindVehicleFormField component.
  • Documentation

    • Updated interfaces for multiple form fields to include new margin properties.

Copy link
Contributor

coderabbitai bot commented Dec 12, 2024

Walkthrough

The pull request introduces a comprehensive update to the application's field components, focusing on adding marginTop and marginBottom properties across multiple UI field components. This change standardizes layout control by allowing more flexible spacing configuration. The modifications span multiple files in the libs/application directory, primarily updating field builders, type definitions, and individual form field components to support dynamic margin settings.

Changes

File Path Change Summary
libs/application/core/src/lib/fieldBuilders.ts Added marginTop and marginBottom to multiple field builder function signatures.
libs/application/types/src/lib/Fields.ts Added marginTop and marginBottom to BaseField interface; removed from specific field interfaces.
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/CheckboxFormField/CheckboxFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/DividerFormField/DividerFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/FileUploadFormField/FileUploadFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx Updated to use Box component with margin properties; refined error handling.
libs/application/ui-fields/src/lib/KeyValueFormField/KeyValueFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/LinkFormField/LinkFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/PaymentChargeOverviewFormField/PaymentChargeOverviewFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/RadioFormField/RadioFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/RedirectToServicePortalFormField/RedirectToServicePortalFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx Updated to include margin properties in the Props interface.
libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx Updated to use Box component with margin properties.
libs/application/ui-fields/src/lib/VehicleRadioFormField/VehicleRadioFormField.tsx Updated to use Box component with margin properties.

Suggested labels

high priority

Suggested reviewers

  • Toti91
  • jonnigs
  • thorkellmani

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82d10de and 8faa9f6.

📒 Files selected for processing (4)
  • libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/LinkFormField/LinkFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/RedirectToServicePortalFormField/RedirectToServicePortalFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx
  • libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx
  • libs/application/ui-fields/src/lib/RedirectToServicePortalFormField/RedirectToServicePortalFormField.tsx
  • libs/application/ui-fields/src/lib/LinkFormField/LinkFormField.tsx

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes missing coverage. Please review.

Project coverage is 35.73%. Comparing base (b7c9767) to head (e1d1582).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
libs/application/core/src/lib/fieldBuilders.ts 0.00% 5 Missing ⚠️
...elds/src/lib/DividerFormField/DividerFormField.tsx 0.00% 2 Missing ⚠️
.../CompanySearchFormField/CompanySearchFormField.tsx 0.00% 1 Missing ⚠️
...iptionFormField/ExpandableDescriptionFormField.tsx 0.00% 1 Missing ⚠️
.../ui-fields/src/lib/LinkFormField/LinkFormField.tsx 0.00% 1 Missing ⚠️
...rtalFormField/RedirectToServicePortalFormField.tsx 50.00% 1 Missing ⚠️
...fields/src/lib/SubmitFormField/SubmitFormField.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17217      +/-   ##
==========================================
- Coverage   35.73%   35.73%   -0.01%     
==========================================
  Files        6941     6941              
  Lines      148401   148407       +6     
  Branches    42334    42340       +6     
==========================================
  Hits        53037    53037              
- Misses      95364    95370       +6     
Flag Coverage Δ
air-discount-scheme-web 0.00% <ø> (ø)
api 3.33% <ø> (ø)
application-api-files 61.87% <ø> (ø)
application-core 75.78% <0.00%> (ø)
application-system-api 38.73% <0.00%> (-0.01%) ⬇️
application-template-api-modules 27.80% <0.00%> (-0.02%) ⬇️
application-templates-accident-notification 28.82% <0.00%> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 25.77% <0.00%> (ø)
application-templates-driving-license 18.16% <0.00%> (ø)
application-templates-estate 13.76% <0.00%> (ø)
application-templates-example-payment 24.72% <0.00%> (ø)
application-templates-financial-aid 14.46% <0.00%> (ø)
application-templates-general-petition 23.15% <0.00%> (ø)
application-templates-inheritance-report 6.59% <0.00%> (ø)
application-templates-marriage-conditions 15.19% <0.00%> (ø)
application-templates-mortgage-certificate 43.64% <0.00%> (ø)
application-templates-parental-leave 29.94% <0.00%> (ø)
application-types 6.51% <ø> (ø)
application-ui-components 1.22% <ø> (ø)
application-ui-shell 22.46% <7.69%> (-0.02%) ⬇️
clients-charge-fjs-v2 28.35% <ø> (ø)
judicial-system-backend 55.90% <ø> (ø)
judicial-system-web 27.88% <ø> (ø)
web 2.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
libs/application/types/src/lib/Fields.ts 100.00% <ø> (ø)
.../lib/AsyncSelectFormField/AsyncSelectFormField.tsx 3.70% <ø> (ø)
...ds/src/lib/CheckboxFormField/CheckboxFormField.tsx 5.00% <ø> (ø)
.../ui-fields/src/lib/DateFormField/DateFormField.tsx 3.12% <ø> (ø)
...elds/src/lib/DisplayFormField/DisplayFormField.tsx 3.70% <ø> (ø)
...rc/lib/FileUploadFormField/FileUploadFormField.tsx 5.00% <ø> (ø)
.../lib/FindVehicleFormField/FindVehicleFormField.tsx 2.17% <ø> (ø)
...ds/src/lib/KeyValueFormField/KeyValueFormField.tsx 8.33% <ø> (ø)
...erviewFormField/PaymentChargeOverviewFormField.tsx 10.00% <ø> (ø)
...i-fields/src/lib/PhoneFormField/PhoneFormField.tsx 5.88% <ø> (ø)
... and 12 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7c9767...e1d1582. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Dec 12, 2024

Datadog Report

All test runs 406b291 🔗

21 Total Test Services: 0 Failed, 20 Passed
🔻 Test Sessions change in coverage: 1 decreased (-0.03%), 92 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-web 0 0 0 2 0 7.81s 1 no change Link
api 0 0 0 4 0 2.49s 1 no change Link
application-api-files 0 0 0 2 0 5.17s 1 no change Link
application-core 0 0 0 97 0 19.71s 1 no change Link
application-system-api 0 0 0 46 0 2m 23.84s 1 no change Link
application-template-api-modules 0 0 0 118 0 2m 32.55s 1 no change Link
application-templates-accident-notification 0 0 0 148 0 14.85s 1 no change Link
application-templates-criminal-record 0 0 0 2 0 9.95s 1 no change Link
application-templates-driving-license 0 0 0 13 0 12.75s 1 no change Link
application-templates-example-payment 0 0 0 2 0 9.88s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (1)

  • application-ui-shell - jest 29.36% (-0.03%) - Details

@HjorturJ HjorturJ marked this pull request as ready for review December 16, 2024 17:48
@HjorturJ HjorturJ requested a review from a team as a code owner December 16, 2024 17:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🔭 Outside diff range comments (1)
libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (1)

Line range hint 91-93: Address TypeScript ignore comment

The code contains a ts-ignore comment that should be addressed to maintain type safety.

Consider:

  1. Adding proper type definitions for the onSelect prop
  2. Documenting why the ts-ignore is necessary if it can't be removed
  3. Creating a follow-up task to resolve the TypeScript error
🧹 Nitpick comments (18)
libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx (1)

21-24: Consider documenting the margin properties in the field interface.

To improve maintainability and developer experience, it would be helpful to add JSDoc comments explaining the margin properties and their default values in the ExpandableDescriptionField interface.

Example addition to the interface:

interface ExpandableDescriptionField {
  /**
   * Top margin of the field container.
   * @default 2
   */
  marginTop?: number;
  
  /**
   * Bottom margin of the field container.
   * @default 2
   */
  marginBottom?: number;
  // ... other properties
}
libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx (1)

41-42: Consider adding a default value for marginBottom

While marginTop has a default fallback of 4, marginBottom doesn't have a default value. This might lead to inconsistent spacing across forms.

-      marginBottom={marginBottom}
+      marginBottom={marginBottom ?? 4}
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (2)

Line range hint 23-46: Consider adding error handling for max value computation

While the computeMax function is well-implemented and memoized, it might benefit from error handling to gracefully handle edge cases where the computation fails or returns invalid values.

 const computeMax = (
   maybeMax: MaybeWithApplicationAndField<number>,
   memoApplication: Application,
   memoField: SliderField,
 ) => {
+  try {
     if (typeof maybeMax === 'function') {
-      return maybeMax(memoApplication, memoField)
+      const result = maybeMax(memoApplication, memoField)
+      if (typeof result !== 'number' || isNaN(result)) {
+        console.warn(`Invalid max value computed for field ${memoField.id}`)
+        return memoField.min || 0
+      }
+      return result
     }
     return maybeMax
+  } catch (error) {
+    console.error(`Error computing max value for field ${memoField.id}:`, error)
+    return memoField.min || 0
+  }
 }

Line range hint 82-89: Consider consolidating value updates

The current implementation calls both onChange and setValue separately with the converted value. Consider consolidating these updates to ensure consistency and prevent potential race conditions.

-              const value = field.saveAsString ? String(val) : val
-              onChange(value)
-              setValue(field.id, value)
+              const newValue = field.saveAsString ? String(val) : val
+              onChange(newValue)
+              // setValue is handled automatically by react-hook-form's Controller
libs/application/ui-fields/src/lib/DividerFormField/DividerFormField.tsx (3)

16-16: Consider adding type safety for margin properties.

The margin properties should be properly typed in the DividerField type to ensure type safety and better IDE support.

Verify that the DividerField type includes these properties:

#!/bin/bash
# Description: Check if DividerField type includes margin properties
ast-grep --pattern 'interface DividerField {
  $$$
  marginTop?: $_
  marginBottom?: $_
  $$$
}'

19-19: Document margin units for better maintainability.

The default margin values (5 and 1) should be documented to clarify the units being used (e.g., spacing units from the design system).

Consider adding a comment:

+  // Margins use spacing units from island-ui/core design system
   <Box marginTop={marginTop ?? 5} marginBottom={marginBottom ?? 1}>

Line range hint 1-43: Implementation aligns well with PR objectives.

The changes successfully implement configurable margins while maintaining component reusability and type safety. The component properly integrates with the design system and follows React/TypeScript best practices.

Consider extracting the default margin values into a shared constants file if they're used across multiple field components, promoting better maintainability and consistency.

libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (1)

Line range hint 38-63: Consider performance and reusability improvements.

The async loading and error handling logic could be optimized:

  1. Extract the loading logic into a reusable custom hook (e.g., useAsyncOptions).
  2. Add loading state management for better UX.
  3. Memoize options transformation to prevent unnecessary re-renders.

Example implementation of a custom hook:

function useAsyncOptions(loadOptions, application, apolloClient) {
  const [state, setState] = useState({
    options: [],
    isLoading: true,
    error: false
  });

  useEffect(() => {
    const load = async () => {
      try {
        setState(prev => ({ ...prev, isLoading: true, error: false }));
        const loaded = await loadOptions({ application, apolloClient });
        setState({ options: loaded, isLoading: false, error: false });
      } catch {
        setState(prev => ({ ...prev, isLoading: false, error: true }));
      }
    };

    load();
  }, [loadOptions]);

  return state;
}
libs/application/ui-fields/src/lib/FileUploadFormField/FileUploadFormField.tsx (1)

51-51: LGTM! Consider documenting margin usage

The Box component implementation with margin props aligns well with the PR's objective of improving form field styling consistency. The component correctly utilizes the island-ui/core design system.

Consider adding a comment or updating the component's documentation to explain the margin props usage and their impact on field spacing.

Also applies to: 85-85

libs/application/ui-fields/src/lib/RedirectToServicePortalFormField/RedirectToServicePortalFormField.tsx (1)

34-35: Consider extracting default margin values to constants

The hardcoded default margin value of 10 should be extracted to a shared constant to maintain consistency across field components.

+ const DEFAULT_MARGIN = 10;

  return (
    <Box
      display="flex"
      alignItems="center"
      justifyContent="center"
      width="full"
-     marginTop={field.marginTop ?? 10}
-     marginBottom={field.marginBottom ?? 10}
+     marginTop={field.marginTop ?? DEFAULT_MARGIN}
+     marginBottom={field.marginBottom ?? DEFAULT_MARGIN}
    >
libs/application/ui-fields/src/lib/KeyValueFormField/KeyValueFormField.tsx (1)

26-27: Ensure consistent margin handling across field components

For consistency with other field components, consider adding default values for marginTop and marginBottom props.

  <Box
    paddingY={field.paddingY}
    paddingX={field.paddingX}
    paddingBottom={field.paddingBottom}
    display={field.display === 'flex' ? 'flex' : 'block'}
    justifyContent="spaceBetween"
-   marginTop={field.marginTop}
-   marginBottom={field.marginBottom}
+   marginTop={field.marginTop ?? DEFAULT_MARGIN}
+   marginBottom={field.marginBottom ?? DEFAULT_MARGIN}
  >
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx (1)

54-54: Consider adding type safety for margin values

The Box component accepts various margin value types (number, array, etc.). Consider adding type constraints to ensure consistent margin value types across the application.

type MarginValue = number | [number, number];

interface Props extends FieldBaseProps {
  field: CompanySearchField & {
    marginTop?: MarginValue;
    marginBottom?: MarginValue;
  };
}
libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx (1)

49-49: Consider margin composition with nested Box components

The component uses nested Box components. While the current implementation works, be mindful of margin composition when both parent and child Box components have margin properties.

Consider documenting the margin composition behavior or establishing guidelines for nested Box components to maintain consistent spacing across the application.

Also applies to: 99-99

libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (1)

52-52: Consider extracting common Box wrapper pattern

Multiple form field components share the same Box wrapper pattern with margin properties.

Consider creating a reusable FormFieldWrapper component to:

  • Reduce code duplication
  • Ensure consistent margin handling
  • Simplify future styling updates
interface FormFieldWrapperProps {
  marginTop?: number;
  marginBottom?: number;
  children: React.ReactNode;
}

const FormFieldWrapper: FC<FormFieldWrapperProps> = ({
  marginTop,
  marginBottom,
  children
}) => (
  <Box marginTop={marginTop} marginBottom={marginBottom}>
    {children}
  </Box>
);

Also applies to: 102-102

libs/application/ui-fields/src/lib/CheckboxFormField/CheckboxFormField.tsx (1)

55-55: Consider adding data-testid attribute for testing

While the Box component implementation is correct for margin handling, consider adding a data-testid attribute to the outer Box for easier testing.

-    <Box marginTop={marginTop} marginBottom={marginBottom}>
+    <Box marginTop={marginTop} marginBottom={marginBottom} data-testid={`${id}-container`}>

Also applies to: 112-112

libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (1)

Line range hint 58-118: Consider extracting error handling logic

While the Box implementation is correct, consider extracting the error handling logic from the onChange handler into a separate function for better maintainability.

+  const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => {
+    if (error) {
+      clearErrors(id)
+    }
+    onChange(e)
+  }
+
   return (
     <Box marginTop={marginTop} marginBottom={marginBottom}>
       {/* ... */}
-          onChange={(e) => {
-            if (error) {
-              clearErrors(id)
-            }
-            onChange(e)
-          }}
+          onChange={handleInputChange}
libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx (1)

360-360: Consider using destructured margin props.

The code uses field.marginTop and field.marginBottom instead of the destructured variables. For consistency with the rest of the codebase, consider using the destructured props.

-    <Box marginTop={field.marginTop} marginBottom={field.marginBottom}>
+    <Box marginTop={marginTop} marginBottom={marginBottom}>
libs/application/types/src/lib/Fields.ts (1)

223-224: LGTM: Well-designed addition of margin properties to BaseField.

The margin properties are correctly added to the BaseField interface, enabling consistent margin handling across all field types through inheritance. The optional nature of these properties maintains backward compatibility.

This change follows good design principles by:

  1. Centralizing margin handling in the base interface
  2. Leveraging the existing BoxProps type from island-ui
  3. Maintaining backward compatibility through optional properties
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 400f28f and 82d10de.

📒 Files selected for processing (22)
  • libs/application/core/src/lib/fieldBuilders.ts (16 hunks)
  • libs/application/types/src/lib/Fields.ts (1 hunks)
  • libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/CheckboxFormField/CheckboxFormField.tsx (3 hunks)
  • libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx (3 hunks)
  • libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/DividerFormField/DividerFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/FileUploadFormField/FileUploadFormField.tsx (3 hunks)
  • libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/KeyValueFormField/KeyValueFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/LinkFormField/LinkFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/PaymentChargeOverviewFormField/PaymentChargeOverviewFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/RadioFormField/RadioFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/RedirectToServicePortalFormField/RedirectToServicePortalFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (3 hunks)
  • libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (1 hunks)
  • libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (2 hunks)
  • libs/application/ui-fields/src/lib/VehicleRadioFormField/VehicleRadioFormField.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • libs/application/ui-fields/src/lib/VehicleRadioFormField/VehicleRadioFormField.tsx
🧰 Additional context used
📓 Path-based instructions (21)
libs/application/ui-fields/src/lib/KeyValueFormField/KeyValueFormField.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/ExpandableDescriptionFormField/ExpandableDescriptionFormField.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/AsyncSelectFormField/AsyncSelectFormField.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/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/RadioFormField/RadioFormField.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/DividerFormField/DividerFormField.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/DateFormField/DateFormField.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/SliderFormField/SliderFormField.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/FileUploadFormField/FileUploadFormField.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/SelectFormField/SelectFormField.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/SubmitFormField/SubmitFormField.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/CheckboxFormField/CheckboxFormField.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/CompanySearchFormField/CompanySearchFormField.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/PaymentChargeOverviewFormField/PaymentChargeOverviewFormField.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/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/ui-fields/src/lib/PhoneFormField/PhoneFormField.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/FindVehicleFormField/FindVehicleFormField.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/RedirectToServicePortalFormField/RedirectToServicePortalFormField.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/LinkFormField/LinkFormField.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/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/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."
🔇 Additional comments (26)
libs/application/ui-fields/src/lib/ExpandableDescriptionFormField/ExpandableDescriptionFormField.tsx (1)

21-24: LGTM! The margin properties implementation looks good.

The changes align well with the PR objectives by allowing consistent form field styling through configurable margins. The nullish coalescing operator ensures a good fallback value of 2 when margins are not specified.

libs/application/ui-fields/src/lib/SubmitFormField/SubmitFormField.tsx (3)

Line range hint 9-12: LGTM: Type definitions are well-structured

The Props interface follows TypeScript best practices and properly extends FieldBaseProps.


19-19: LGTM: Clean prop destructuring

The margin properties are properly destructured alongside existing field properties.


Line range hint 1-54: LGTM: Component follows best practices for reusability

The implementation:

  • Uses proper TypeScript features
  • Follows modular design principles
  • Implements proper tree-shaking through specific imports
  • Uses shared UI components effectively
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (2)

Line range hint 1-14: Well-structured TypeScript implementation!

The component demonstrates excellent TypeScript practices with proper type definitions, generics usage, and type safety. The shared types from @island.is/application/types promote reusability across the application system.

Also applies to: 16-21


47-47: Margin properties implementation aligns with PR objectives

The addition of marginTop and marginBottom props to the Box component successfully implements the requested styling consistency while maintaining type safety through the field object.

libs/application/ui-fields/src/lib/LinkFormField/LinkFormField.tsx (2)

Line range hint 8-12: LGTM! Type definitions follow best practices.

The component demonstrates good TypeScript usage with clear prop types, enabling reusability and type safety.


40-43: LGTM! Margin properties enhance styling flexibility.

The changes align with the PR's objective of improving form field styling consistency by allowing granular margin control.

libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (1)

64-64: Verify type definitions for margin properties.

The Box component usage with margin properties aligns well with the PR objectives for consistent form field styling. However, let's ensure proper type safety.

Also applies to: 116-116

libs/application/ui-fields/src/lib/FileUploadFormField/FileUploadFormField.tsx (1)

Line range hint 1-87: Implementation follows best practices

The component implementation adheres to all required guidelines:

  • Proper TypeScript usage with well-defined props and types
  • Positioned in libs directory for reusability across NextJS apps
  • Specific imports supporting effective tree-shaking
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx (1)

35-36: 🛠️ Refactor suggestion

Standardize margin handling with other field components

The margin implementation differs from other field components:

  1. marginTop uses responsive array values [2, 4]
  2. marginBottom lacks a default value

Consider standardizing the margin implementation across all field components.

Let's verify the margin implementation across other field components:

libs/application/ui-fields/src/lib/DisplayFormField/DisplayFormField.tsx (1)

45-46: Verify type definitions for margin properties

The margin properties are correctly implemented using the Box component. However, we should verify the type definitions to ensure type safety.

✅ Verification successful

Based on the extensive search results, I can see that the margin properties are widely used throughout the codebase with the Box component from island-ui/core, but I don't see any explicit type definitions for the field object that would include marginTop and marginBottom properties. However, since these properties are being used with the Box component, which is a common UI component that accepts margin props, and the implementation follows a consistent pattern across the codebase, this is a safe usage.

The margin properties are correctly implemented

The marginTop and marginBottom properties are being used appropriately with the Box component from island-ui/core, which is designed to handle these spacing properties. While we don't see the explicit type definition, this is a common pattern in the codebase for handling component spacing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for BaseField or DisplayField type definitions
ast-grep --pattern 'interface $field {
  $$$
  marginTop?: $_;
  marginBottom?: $_;
  $$$
}'

Length of output: 94


Script:

#!/bin/bash
# Let's try a broader search for type definitions related to form fields
ast-grep --pattern 'type $field = {
  $$$
  margin$_?: $_;
  $$$
}'

# Also search for interface definitions with a different pattern
ast-grep --pattern 'interface $_ {
  $$$
  margin$_?: $_;
  $$$
}'

# Search for actual usage of these properties in the codebase
rg "marginTop|marginBottom" -A 2 -B 2

Length of output: 67296


Script:

#!/bin/bash
# Let's search for field type definitions in the DisplayFormField component and related files
rg -l "DisplayFormField" --type ts --type tsx

# Also search for any field interface or type definitions that might contain margin properties
ast-grep --pattern 'type $field = {
  $$$
  margin?: $_;
  $$$
}'

ast-grep --pattern 'interface $field {
  $$$
  margin?: $_;
  $$$
}'

Length of output: 212


Script:

#!/bin/bash
# Search for DisplayFormField files and their content
rg "DisplayFormField" -A 5 -B 5

# Search for field type definitions with a broader pattern
ast-grep --pattern 'type $_ = {
  $$$
}'

# Search for field interfaces with a broader pattern
ast-grep --pattern 'interface $_ {
  $$$
}'

Length of output: 65756

libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx (1)

41-42: LGTM: Props interface correctly extended

The margin properties are properly typed in the Props interface.

libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (1)

41-42: LGTM: Consistent margin property implementation

The margin properties are implemented consistently with other form field components.

libs/application/ui-fields/src/lib/CheckboxFormField/CheckboxFormField.tsx (1)

44-45: LGTM: Props properly extended with margin properties

The margin properties are correctly added to the field destructuring, maintaining type safety and component consistency.

libs/application/ui-fields/src/lib/PaymentChargeOverviewFormField/PaymentChargeOverviewFormField.tsx (1)

50-50: LGTM: Consistent margin implementation

The Box component implementation follows the established pattern for margin handling across form fields.

libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (1)

50-51: LGTM: Margin properties properly integrated

The margin properties are correctly added to the field destructuring, maintaining consistency with other form fields.

libs/application/ui-fields/src/lib/RadioFormField/RadioFormField.tsx (2)

43-44: LGTM! Props added for consistent margin control.

The addition of marginTop and marginBottom props aligns with the PR's objective of improving form field styling consistency.


54-60: LGTM! Box component implementation enhances layout control.

The Box component usage with margin props provides consistent spacing control while maintaining accessibility with proper ARIA attributes.

libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx (2)

48-49: LGTM! Props added for consistent margin control.

The addition of margin props aligns with the standardization of form field styling.


120-120: LGTM! Box component implementation enhances layout control.

The Box component usage with margin props provides consistent spacing control.

Also applies to: 160-160

libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx (1)

162-163: LGTM! Props added for consistent margin control.

The addition of margin props aligns with the standardization of form field styling.

libs/application/core/src/lib/fieldBuilders.ts (4)

76-77: LGTM: Common field extraction properly handles margin properties.

The margin properties are correctly extracted and propagated, ensuring consistent margin handling across all field types.

Also applies to: 91-92


403-404: LGTM: Divider field correctly implements margin properties.

The margin properties are properly typed and integrated into the divider field builder.

Also applies to: 406-406, 416-417


470-471: LGTM: Submit field properly implements margin properties.

The margin properties are correctly integrated into the submit field builder, maintaining consistency with other field types.

Also applies to: 481-482, 495-496


949-950: LGTM: Slider field correctly implements margin properties.

The margin properties are properly integrated into the slider field builder, maintaining consistency with other field types.

Also applies to: 977-978

Copy link
Member

@norda-gunni norda-gunni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some style questions. Feel completely free to just resolve them if you disagree and I'll be happy to approve.

Copy link
Member

@norda-gunni norda-gunni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@HjorturJ HjorturJ added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Dec 17, 2024
@kodiakhq kodiakhq bot merged commit d472db7 into main Dec 17, 2024
97 checks passed
@kodiakhq kodiakhq bot deleted the chore/form-field-styling branch December 17, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated:automerge (Disabled) Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants