-
Notifications
You must be signed in to change notification settings - Fork 0
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(form): add setValue
method to FormGroup class
#191
Conversation
❌ Deploy Preview for astro-reactive failed.
|
✅ Deploy Preview for astro-reactive-docs canceled.
|
Hi @NeilAn99! Thank you for this PR, it looks like you got the solution. There are just some small typescript hiccups 🙂 Can you run |
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.
Hi @NeilAn99! I just have a couple of suggestions for improvements :)
@@ -7,6 +7,7 @@ import Form, { | |||
} from "@astro-reactive/form"; | |||
import { Validators } from "@astro-reactive/validator"; | |||
import type { Submit } from "@astro-reactive/common"; | |||
import { info } from "astro/dist/core/logger/core"; |
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.
I think you can remove this now :)
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.
removed!
packages/form/core/form-group.ts
Outdated
setValue(values: object) { | ||
this.controls.map((control) => { | ||
for (const value in values) { | ||
if (value == control.name) { | ||
control.setValue(values[value]); | ||
} | ||
} | ||
}); | ||
} |
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.
You got the gist of the solution! However, we can still improve this, performance-wise.
Usually, you want to avoid having a loop within a loop for performance. 😉
Try to do this by just looping through the names in your values object and using the FormGroup.get function to get the appropriate control.
Something like:
Object.keys(values).forEach(name => this.get(name)?.setValue(values[name]));
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.
Changed the solution. Thanks for the great suggestion.
Made some new commits, let me know if there's anything else I can improve. |
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.
Alright, this looks good! Thanks @NeilAn99! 🚀
* feat: install and setup turborepo * chore: update npm scripts to use turbo * chore: add clean scripts * chore: add eslint-config-turbo and update workspace deps * devops: test commit to trigger landing page build and deploy * fix(script): use rimraf instead of 'rm -rf' * chore(docs): change site option (#198) * chore: add test:watch script to turbo * docs: add developing guide * feat(form): add `setValue` method to FormGroup class (#191) * add mastodon ownership verification * chore: change package version to experimental * fix: fix packages/common access Co-authored-by: Ayo <[email protected]> Co-authored-by: Woramat Ngamkham <[email protected]> Co-authored-by: Neil An <[email protected]>
* feat: install and setup turborepo * chore: update npm scripts to use turbo * chore: add clean scripts * chore: add eslint-config-turbo and update workspace deps * devops: test commit to trigger landing page build and deploy * fix(script): use rimraf instead of 'rm -rf' * chore(docs): change site option (#198) * chore: add test:watch script to turbo * docs: add developing guide * feat(form): add `setValue` method to FormGroup class (#191) * add mastodon ownership verification * chore: change package version to experimental * fix: fix packages/common access Co-authored-by: Ayo <[email protected]> Co-authored-by: Woramat Ngamkham <[email protected]> Co-authored-by: Neil An <[email protected]>
feat(form): add
setValue
method to FormGroup classFixes #175
Description of changes:
setValue
method in the FormGroup class which will set the values for any FormControls in the FormGroup.setValue
method. For example the following code:will produce the following page:
Tag a reviewer: @ayoayco
Tasks:
npm run build
npm run test
npm run lint