feat: implement a very basic MVP form element #206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces a basic implementation of the Sequence diagram for form submission flowsequenceDiagram
participant User
participant Form as AuroForm
participant FormElements as Form Elements
participant Event as Event System
User->>FormElements: Inputs data
FormElements->>Form: input event
Form->>Form: Update _formState
FormElements->>Form: auroFormElement-validated event
Form->>Form: Update validity state
User->>Form: Clicks submit
Form->>Event: Dispatch submit event with form value
Class diagram for AuroForm componentclassDiagram
class AuroForm {
-_formState: FormState
+static formElementTags: string[]
+static submitElementTags: string[]
+value: Record~string, any~
+validity: string
+isInitialState: boolean
+isFormElement(element: HTMLElement): boolean
+isSubmitElement(element: HTMLElement): boolean
+getSubmitFunction(): function
+queryAuroElements(): NodeList
+onSlotChange(): void
}
note for AuroForm "New form component that manages form state and validation"
State diagram for form validationstateDiagram-v2
[*] --> Initial: Form Created
Initial --> Valid: All required fields valid
Initial --> Invalid: Required fields missing/invalid
Valid --> Invalid: Field becomes invalid
Invalid --> Valid: All validations pass
Valid --> [*]: Submit form
note right of Invalid: Form tracks validity
of all child elements
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Not feature complete, but ready to start implementing real work.
This listens to, and attaches event listeners for, the following: - input - auroFormElement-validated - button[type=submit] clicks It also adds a datepicker to the example form.
1. Uses one root querySelector instead of looping through elements. 2. Add more explicit validity state handling. 3. Add handling for renamed auro components. 4. Revert import in datepicker html example file.
Remove console logs, fix elementCollection tag name casing, dispatch submit event on submit, add renamed formElement `name` condition.
Formkit prefixes were causing issues with installation after rebasing.
DukeFerdinand
force-pushed
the
dhook/auro-form
branch
from
a916fb8 to
73a3e60
Compare
jason-capsule42
approved these changes
Jan 17, 2025
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @DukeFerdinand - I've reviewed your changes - here's some feedback:
Overall Comments:
- Remove the debug output (
<p>Value: ${JSON.stringify(this.value)}</p>and<p>Validity: ${this.validity}</p>) from the render method as this shouldn't be in production code. Consider adding a debug mode if this visibility is needed for development. - The validation logic could be simplified and improved. Consider adding proper error states and messages rather than just returning 'valid'/'invalid'. This would provide better UX for form validation.
Here's what I looked at during the review
- 🟡 General issues: 6 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.
Comment on lines
+192
to
+194
| if (this.isFormElement(eventTarget)) { | ||
| this._formState[eventTarget.getAttribute("name")].value = eventTarget.value; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Add a guard clause to check for name attribute before accessing formState
The current code assumes the name attribute exists on form elements. Add a check like if (this.isFormElement(eventTarget) && eventTarget.hasAttribute('name')) to prevent potential errors.
Suggested change
| if (this.isFormElement(eventTarget)) { | |
| this._formState[eventTarget.getAttribute("name")].value = eventTarget.value; | |
| } | |
| if (this.isFormElement(eventTarget) && eventTarget.hasAttribute('name')) { | |
| this._formState[eventTarget.getAttribute("name")].value = eventTarget.value; | |
| } |
There was a problem hiding this comment.
will handle this in subsequent work (in the next couple of hours)
|
|
||
| // this property is DEMO ONLY! Please delete. | ||
| cssClass: { type: String } | ||
| _formState: { attribute: false }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
issue (performance): Store submit handler as instance property to prevent memory leaks
Create a class property _submitHandler in constructor and assign getSubmitFunction() result to it. This ensures proper event listener cleanup.
|
🎉 This PR is included in version 2.0.0-beta.16 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not feature complete, but ready to start implementing real work.
Alaska Airlines Pull Request
Closes #185
Before Submitting this pull request:
Developmentsectionnote: all pull requests require at least one linked ticket
Ready For Review, all ticket's linked underDevelopmentmust have their status changed toReady For Reviewas wellBy submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I have performed a self-review of my own update.
Summary by Sourcery
Implement a basic MVP form element.
New Features:
auro-formelement to provide a basic form structure. Include form submission and validation capabilities.Tests: