nuxt / ui

A UI Library for Modern Web Apps, powered by Vue & Tailwind CSS.
https://ui.nuxt.com
MIT License
3.45k stars 383 forks source link

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

Closed romhml closed 9 months ago

romhml commented 10 months ago

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

vercel[bot] commented 10 months ago

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[bot] commented 10 months ago

Live Preview ready!

Name Edit Preview Latest Commit
ui Edit on Studio ↗︎ View Live Preview 9d4620ffc8909a0ce6f9b0f0aacc98b71138e71b
romhml commented 10 months ago

@benjamincanac I merged this branch with https://github.com/nuxtlabs/ui/pull/491 to fix conflicts and make it easier to test.

benjamincanac commented 10 months ago

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

romhml commented 10 months ago

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

romhml commented 10 months ago

@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?

benjamincanac commented 10 months ago

What do you mean by exposed attributes?

romhml commented 10 months ago

The Form's public functions here: https://github.com/nuxtlabs/ui/pull/487/files#diff-c923347138b9a573e905bb6bdffe4e52ba34f7f220fdadf5b5c4011cf73298a9R100-R125

benjamincanac commented 10 months ago

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

romhml commented 10 months ago

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)
benjamincanac commented 10 months ago

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

romhml commented 10 months ago

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

benjamincanac commented 9 months ago

@romhml I've updated the resolves in the pr description, can you validate them? 😊

romhml commented 9 months ago

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.

romhml commented 9 months ago

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

benjamincanac commented 9 months ago

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 commented 9 months ago

Looks good! Thanks!

benjamincanac commented 9 months ago

@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 commented 9 months ago

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

benjamincanac commented 9 months ago

Can I let you improve it? 😊

romhml commented 9 months ago

@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.