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

feat(form): add setValue method to FormGroup class #191

Merged
merged 4 commits into from
Nov 12, 2022

Conversation

NeilAn99
Copy link
Contributor

feat(form): add setValue method to FormGroup class

Fixes #175

Description of changes:

  • Created a new setValue method in the FormGroup class which will set the values for any FormControls in the FormGroup.
  • Updated the job application demo page to use the new setValue method. For example the following code:
    image
    will produce the following page:
    image

Tag a reviewer: @ayoayco

Tasks:

  • I have ran the build command to make sure apps are working: npm run build
  • I have ran the tests to make sure nothing is broken: npm run test
  • I have ran the linter to make sure code is clean: npm run lint
  • I have reviewed my own code to remove changes that are not needed

@netlify
Copy link

netlify bot commented Nov 12, 2022

Deploy Preview for astro-reactive failed.

Name Link
🔨 Latest commit 41069bf
🔍 Latest deploy log https://app.netlify.com/sites/astro-reactive/deploys/63700100fe555e0008743b22

@netlify
Copy link

netlify bot commented Nov 12, 2022

Deploy Preview for astro-reactive-docs canceled.

Name Link
🔨 Latest commit 41069bf
🔍 Latest deploy log https://app.netlify.com/sites/astro-reactive-docs/deploys/63700100e01fc90008958dc9

@ayoayco
Copy link
Member

ayoayco commented Nov 12, 2022

Hi @NeilAn99! Thank you for this PR, it looks like you got the solution. There are just some small typescript hiccups 🙂

Can you run npm run check in the project root directory and see where it fails?

Copy link
Member

@ayoayco ayoayco left a 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";
Copy link
Member

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

Comment on lines 22 to 30
setValue(values: object) {
this.controls.map((control) => {
for (const value in values) {
if (value == control.name) {
control.setValue(values[value]);
}
}
});
}
Copy link
Member

@ayoayco ayoayco Nov 12, 2022

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]));

Copy link
Contributor Author

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.

@NeilAn99
Copy link
Contributor Author

Made some new commits, let me know if there's anything else I can improve.

Copy link
Member

@ayoayco ayoayco left a 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! 🚀

@ayoayco ayoayco merged commit 31884a1 into astro-reactive:main Nov 12, 2022
ayoayco added a commit that referenced this pull request Nov 16, 2022
* 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]>
ayoayco added a commit that referenced this pull request Nov 23, 2022
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(form): add setValue method to FormGroup class
2 participants