-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(application-system): Add dependent loading to async select #17525
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 eslint@8.57.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces an Changes
Sequence DiagramsequenceDiagram
participant User
participant PrimarySelect
participant DependentSelect
participant DataSource
User->>PrimarySelect: Select primary value
PrimarySelect->>DependentSelect: Trigger update
DependentSelect->>DataSource: Fetch options
DataSource-->>DependentSelect: Return filtered options
DependentSelect->>User: Display updated options
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)libs/application/core/src/lib/fieldBuilders.ts (1)Pattern
libs/application/types/src/lib/Fields.ts (1)Pattern
🔇 Additional comments (3)
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (1)
libs/application/types/src/lib/Fields.ts (1)
392-392
: Consider improving type safety forupdateOnSelect
.While the implementation is correct, we could enhance type safety by ensuring the field IDs in
updateOnSelect
array actually exist.Consider using a generic type parameter to constrain the field IDs:
export interface AsyncSelectField<T extends string = string> extends InputField { // ... other properties updateOnSelect?: T[] loadOptions(c: AsyncSelectContext): Promise<Option[]> }Also applies to: 398-398
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/application/core/src/lib/fieldBuilders.ts
(2 hunks)libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/asyncSelectSubsection.ts
(2 hunks)libs/application/types/src/lib/Fields.ts
(2 hunks)libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx
(2 hunks)libs/shared/form-fields/src/lib/SelectController/SelectController.tsx
(1 hunks)libs/shared/utils/src/lib/clearInputsOnChange.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
libs/shared/utils/src/lib/clearInputsOnChange.ts (2)
Pattern libs/shared/**/*
: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/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/templates/reference-template/src/forms/exampleForm/simpleInputsSection/asyncSelectSubsection.ts (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/shared/form-fields/src/lib/SelectController/SelectController.tsx (2)
Pattern libs/shared/**/*
: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/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/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."
🔇 Additional comments (5)
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (1)
71-77
: Ensure 'watchUpdateOnSelect' is handled correctlyAfter adjusting
watchUpdateOnSelect
to always be an array, the checks in thisuseEffect
will function as intended. Make sure that the indexing and length checks align with the updated type ofwatchUpdateOnSelect
.libs/shared/utils/src/lib/clearInputsOnChange.ts (1)
6-6
: LGTM!Changing the cleared value to
null
ensures consistency with theSelectController
component, which sets unselected values tonull
. This promotes better handling of empty states.libs/application/types/src/lib/Fields.ts (1)
53-57
: LGTM! Well-structured type definition.The new
AsyncSelectContext
type properly extends the base context with the additionalselectedValue
property needed for dependent loading.libs/application/core/src/lib/fieldBuilders.ts (1)
244-244
: LGTM! Clean implementation.The
updateOnSelect
property is correctly extracted and included in the returned object, maintaining consistency with the existing pattern.Also applies to: 259-259
libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/asyncSelectSubsection.ts (1)
76-84
: LGTM! Clear and informative field description.The description field effectively explains the purpose of the dependent async select feature to users.
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx
Show resolved
Hide resolved
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx
Show resolved
Hide resolved
...plates/reference-template/src/forms/exampleForm/simpleInputsSection/asyncSelectSubsection.ts
Show resolved
Hide resolved
...plates/reference-template/src/forms/exampleForm/simpleInputsSection/asyncSelectSubsection.ts
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core file changes LGTM
...
Attach a link to issue if relevant
What
Enable async selects to fetch their values using values selected in other async select fields
Why
To enable more responsive and user friendly applications by avoiding the need for splitting such functionality into multiple screens.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
updateOnSelect
property for async select fields.Improvements
null
instead of an empty string.Bug Fixes