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): improve form control and input validation trigger #487

Merged
merged 26 commits into from
Aug 12, 2023
Merged

feat(Form): improve form control and input validation trigger #487

merged 26 commits into from
Aug 12, 2023

Conversation

romhml
Copy link
Collaborator

@romhml romhml commented Aug 3, 2023

This PR aims to give more control over the form component and improve input validation. It features:

  • triggering validation on input events.
  • a new prop validate-on to specify when validation should happen
  • a function to clear form validation errors.
  • a function to manually set errors.
  • an example with back-end validation errors.

@vercel
Copy link

vercel bot commented Aug 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview Aug 12, 2023 11:48am

@nuxt-studio
Copy link

nuxt-studio bot commented Aug 3, 2023

Live Preview ready!

Name Edit Preview Latest Commit
ui Edit on Studio ↗︎ View Live Preview 9d4620f

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

@benjamincanac I merged this branch with #491 to fix conflicts and make it easier to test.

Copy link
Member

@romhml Do you need me to wait on merging #496 and #497 as you're in the middle of a large refactor?

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

No you can go ahead, it should be easy to merge

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

@benjamincanac I'm almost done with the changes, I just need to improve the documentation. Do you know if it's possible to replicate the ComponentProps layout for exposed attributes?

Copy link
Member

What do you mean by exposed attributes?

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

Copy link
Member

Have you tried setting componentMeta.metaFields.exposed to true in docs/nuxt.config.ts?

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

Just tried it, it breaks everything:

[nuxt-component-meta 2:29:20 PM] ✔ Components metas parsed in 3444.11ms

[2:29:40 PM]  ERROR  Cannot start nuxt:  Invalid string length

  at JSON.stringify (<anonymous>)
  at getStringifiedComponents (node_modules/.pnpm/nuxt-component-meta@0.5.3_rollup@3.26.2/node_modules/nuxt-component-meta/dist/parser.mjs:62:47)
  at getVirtualModuleContent (node_modules/.pnpm/nuxt-component-meta@0.5.3_rollup@3.26.2/node_modules/nuxt-component-meta/dist/parser.mjs:63:59)
  at Object.updateOutput (node_modules/.pnpm/nuxt-component-meta@0.5.3_rollup@3.26.2/node_modules/nuxt-component-meta/dist/parser.mjs:89:18)
  at async Context.buildStart (node_modules/.pnpm/nuxt-component-meta@0.5.3_rollup@3.26.2/node_modules/nuxt-component-meta/dist/module.mjs:24:9)
  at async Promise.all (index 8)
  at async hookParallel (node_modules/.pnpm/vite@4.3.9_@types+node@20.4.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42512:9)
  at async Object.buildStart (node_modules/.pnpm/vite@4.3.9_@types+node@20.4.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42824:13)
  at async node_modules/.pnpm/vite@4.3.9_@types+node@20.4.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:63536:13
  at async _createServer (node_modules/.pnpm/vite@4.3.9_@types+node@20.4.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:63566:9)

Copy link
Member

Mmmh, it might be easier to document this manually then.

@romhml
Copy link
Collaborator Author

romhml commented Aug 4, 2023

@benjamincanac the PR is ready. It incorporates significant refactoring and simplifications, but I've made sure to maintain compatibility with previous examples.

Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

I've put some comments for typos and some questions but everything else looks perfect, great job 🚀

@@ -44,11 +44,11 @@ const schema = z.object({

type Schema = z.infer<typeof schema>

const form = ref<Form<Schema>>()
const form = ref()
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's no longer needed since the example does not rely on form.value.validate now that validation happens automatically on submit. It was required on previous examples to get the right return type.

The ref itself is also useless, I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe to type the clear method?

docs/components/content/examples/FormExampleJoi.vue Outdated Show resolved Hide resolved
docs/components/content/examples/FormExampleYup.vue Outdated Show resolved Hide resolved
docs/components/content/examples/FormExampleZod.vue Outdated Show resolved Hide resolved
docs/components/content/examples/FormExampleZod.vue Outdated Show resolved Hide resolved
docs/content/3.forms/10.form.md Outdated Show resolved Hide resolved
docs/content/3.forms/10.form.md Show resolved Hide resolved
docs/content/3.forms/10.form.md Show resolved Hide resolved
docs/content/3.forms/10.form.md Show resolved Hide resolved
const ui = computed<Partial<typeof appConfig.ui.formGroup>>(() => defu({}, props.ui, appConfig.ui.formGroup))

const formErrors = inject<Ref<FormError[]> | null>('form-errors', null)
const errorMessage = computed(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just not call it error here as it's provided this way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed it do avoid dupe keys warning in the return statement, we can rename it to error if you prefer

@romhml
Copy link
Collaborator Author

romhml commented Aug 11, 2023

@benjamincanac I applied your suggestions and I confirm that all the issues you mentioned will be solved by this PR.

@benjamincanac
Copy link
Member

Thanks! I'll have a look in the afternoon to wrap everything up. Sorry for the delay I did not have a lot on of time this week.

No worries at all! Thanks a lot for the changes 😊

I've updated the API section if you want to take a look!

@romhml
Copy link
Collaborator Author

romhml commented Aug 11, 2023

Looks good! Thanks!

@benjamincanac
Copy link
Member

@romhml Not sure about my last commit: caebf48 (#487), there was an error when clicking any checkbox in the documentation (to toggle the props). Do we need to make the name prop required?

@romhml
Copy link
Collaborator Author

romhml commented Aug 11, 2023

It might be better to emit the event only if the path is provided in emitFormEvent

@benjamincanac
Copy link
Member

Can I let you improve it? 😊

@romhml
Copy link
Collaborator Author

romhml commented Aug 12, 2023

@benjamincanac Never mind what I had in mind did not work. Your changes are good to go as is, no need for requiring the name attribute since it's only needed if used with the form component.

provide('form-group', {
error,
name: computed(() => props.name),
size: computed(() => props.size)
Copy link
Member

Choose a reason for hiding this comment

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

@romhml FYI I've made a small change here to only proxy props.size and not props.size ?? appConfig.ui.input.default.size as a SelectMenu, Range, etc. might have a different default than the input one.

@benjamincanac benjamincanac merged commit f87ae62 into nuxt:dev Aug 12, 2023
2 checks passed
This was referenced Aug 12, 2023
benjamincanac added a commit that referenced this pull request Sep 7, 2023
Co-authored-by: Benjamin Canac <canacb1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants