Rework design form functionality #395
Conversation
|
Reviewer's Guide by SourceryThis pull request refactors the form component to use a more modular approach. It removes the fields property and instead uses the children of the form component to render the form fields. It also adds support for nested forms and arrays. Sequence diagram for form submission flowsequenceDiagram
participant User
participant Form as EccUtilsDesignForm
participant Input as EccUtilsDesignFormInput
participant Group as EccUtilsDesignFormGroup
User->>Form: Submit form
Form->>Form: handleSubmit()
Form->>Form: Check validity
alt form is valid
Form->>Form: Set loading state
Form-->>User: Show loading state
Form->>Form: Emit submit event
Form-->>User: Show success message
else form is invalid
Form-->>User: Show validation errors
end
Class diagram showing the new form component structureclassDiagram
class EccUtilsDesignForm {
-form: object
-formState: string
-canSubmit: boolean
-submitDisabledByUser: boolean
-errorMessage: string
-successMessage: string
-items: Array~Element~
+firstUpdated()
+handleSubmit()
+render()
}
class EccUtilsDesignFormInput {
+label: string
+key: string
+type: FormItemType
+disabled: boolean
+tooltip: string
+required: boolean
+value: any
-path: string
+handleValueUpdate()
+handleFileUpload()
+render()
}
class EccUtilsDesignFormGroup {
+label: string
+key: string
+type: string
+required: boolean
+tooltip: string
+instances: number
-arrayInstances: Array
-items: Array~Element~
+render()
}
EccUtilsDesignForm *-- EccUtilsDesignFormInput
EccUtilsDesignForm *-- EccUtilsDesignFormGroup
State diagram for form component statesstateDiagram-v2
[*] --> Idle
Idle --> Loading: Submit form
Loading --> Success: Submit successful
Loading --> Error: Submit failed
Success --> Idle: Reset
Error --> Idle: Reset
state Idle {
[*] --> Valid
Valid --> Invalid: Validation failed
Invalid --> Valid: Validation passed
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @SalihuDickson - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating duplicate utility functions (e.g. renderInTooltip, formatLabel) into a shared utilities module to improve maintainability
- Error handling approach varies between components - recommend standardizing error handling and validation patterns across the codebase
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -83,499 +84,32 @@ export default class EccUtilsDesignForm extends LitElement { | |||
| formStyles, | |||
| ]; | |||
|
|
|||
| @property({ type: Array, reflect: true }) fields: Array<Field> = []; | |||
| @state() private form: object = {}; | |||
There was a problem hiding this comment.
suggestion (bug_risk): Consider initializing canSubmit to false by default and enabling it only after validating form state
The current implementation may allow submission of invalid forms. Consider restoring form validation logic or documenting why immediate submission should be allowed.
Suggested implementation:
@state() private canSubmit = false; @state() private form: object = {};
private validateForm(): void {
// Check if all required fields are filled
const hasEmptyRequired = this.requiredButEmpty.length > 0;
// Update canSubmit based on validation
this.canSubmit = !hasEmptyRequired && Object.keys(this.form).length > 0;
}
protected updated(changedProperties: Map<string, unknown>): void {
if (changedProperties.has('form') || changedProperties.has('requiredButEmpty')) {
this.validateForm();
}
}Note: You may need to:
- Adjust the validation logic in
validateForm()if there are additional validation requirements specific to your forms - Ensure the
updatedlifecycle method properly merges with any existing implementation if it already exists
| @@ -644,6 +178,7 @@ export default class EccUtilsDesignForm extends LitElement { | |||
|
|
|||
| private handleSubmit(e: Event) { | |||
There was a problem hiding this comment.
suggestion: The error handling in handleSubmit could be improved to provide better feedback
Consider adding more specific error handling and user feedback when form validation fails.
Suggested implementation:
private handleSubmit(e: Event) {
e.preventDefault();
try {
// Check for required fields
const emptyRequired = this.items
.filter((item: any) => item.hasAttribute('required') && !item.value)
.map((item: any) => item.name || 'Unknown field');
if (emptyRequired.length > 0) {
this.formState = "error";
this.errorMessage = `Please fill in required fields: ${emptyRequired.join(', ')}`;
this.requiredButEmpty = emptyRequired;
return;
}
// Check for invalid fields
const invalidFields = this.items
.filter((item: any) => !item.checkValidity())
.map((item: any) => item.name || 'Unknown field');
if (invalidFields.length > 0) {
this.formState = "error";
this.errorMessage = `Please correct invalid fields: ${invalidFields.join(', ')}`;
return;
}
To fully implement this change, you'll also need to:
- Add error message display in the template, likely using sl-alert for errors
- Add CSS styles for highlighting invalid fields
- Ensure the rest of the handleSubmit method (which I can't see) properly sets formState back to "idle" after successful submission
- Consider adding field-level error messages using the sl-input or equivalent component's error states
| @@ -1,13 +1,12 @@ | |||
| # ecc-utils-design | |||
|
|
|||
| The `@elixir-cloud/design` package is a foundational utility library that powers the ELIXIR Cloud Component's (ECC) ecosystem. | |||
| The `@elixir-cloud/design` package is a foundational utility library that powers the ELIXIR Cloud Component's (ECC) ecosystem. | |||
There was a problem hiding this comment.
issue (typo): Typo: "Component's" should be "Components"
The possessive should be plural: "ELIXIR Cloud Components".
| The `@elixir-cloud/design` package is a foundational utility library that powers the ELIXIR Cloud Component's (ECC) ecosystem. | |
| The `@elixir-cloud/design` package is a foundational utility library that powers the ELIXIR Cloud Components (ECC) ecosystem. |
| import "@shoelace-style/shoelace/dist/components/tooltip/tooltip.js"; | ||
|
|
||
| export const getListData = (input: string) => { | ||
| if (typeof input !== "string") return input; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (typeof input !== "string") return input; | |
| if (typeof input !== "string") { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| private findNearestFormGroup(element: HTMLElement | null = this): void { | ||
| if (!element) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!element) return; | |
| if (!element) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| private findNearestFormGroup(element: HTMLElement | null = this): void { | ||
| if (!element) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!element) return; | |
| if (!element) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| } | ||
|
|
||
| const { parentElement } = element; | ||
| if (!parentElement) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!parentElement) return; | |
| if (!parentElement) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| private renderTemplate(): TemplateResult { | ||
| const { type } = this; | ||
| if (type === "switch") return this.renderSwitchTemplate(); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (type === "switch") return this.renderSwitchTemplate(); | |
| if (type === "switch") { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| private renderTemplate(): TemplateResult { | ||
| const { type } = this; | ||
| if (type === "switch") return this.renderSwitchTemplate(); | ||
| if (type === "file") return this.renderFileTemplate(); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (type === "file") return this.renderFileTemplate(); | |
| if (type === "file") { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| const { type } = this; | ||
| if (type === "switch") return this.renderSwitchTemplate(); | ||
| if (type === "file") return this.renderFileTemplate(); | ||
| if (type === "select") return this.renderSelectTemplate(); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (type === "select") return this.renderSelectTemplate(); | |
| if (type === "select") { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
@anuragxxd, can you please take a quick look at the demo of the form component in this PR and tell me what you think. |
SalihuDickson
force-pushed
the
rework-design-form-functionality
branch
from
23053f3 to
9bae575
Compare
Description
This PR refactors the functionality of the
utils-design-formcomponent, addressing the issues outlined in #392.Comparing Both Approaches
Currently, building a simple form component follows this approach:
At first glance, this configuration-driven approach appears straightforward. However, as forms become more complex, the configuration can quickly become bulky and difficult to manage.
Event Handling
For example, since all fields exist within a single component, event handling must be centralized. While this isn't an issue for simple fields, it becomes problematic in more intricate configurations.
In this example, fields are deeply nested. To listen for a change event on the first street field, we need a structured key like address.details[0].street to properly track its position within the form. This adds complexity, making it harder for developers to ensure they’re handling the correct event.
The Proposed Solution
To simplify event handling and improve maintainability, this PR introduces a new approach:
With this approach:
This PR simplifies form management while maintaining flexibility, making it easier to work with complex configurations.
Custom Elements Integration
This Approach Also allows for better custom element integration. By separating the individual components in this way, we can allow more technically inclined users to integrate their own elements and better handle specialized scenarios.
Let's take a look at an informative example.
Currently, our form component comes with a submit button, nothing too fancy, but we also provide some limited options to allow developers style the button. But sometimes, this is not enough for some developers. Considering that, the proposed approach allows developers to replace this button with a custom button of their own, like this;
Or for example, I want to use a custom input field in my form, my proposed approach still gives the form component the ability to track custom input fields and include them in the form data. For example;
In this example the form will still track the
TextFieldcomponent with the ecc-key value.