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: add vee-validate #85

Merged
merged 20 commits into from
Oct 6, 2023
Merged

Conversation

sadeghbarati
Copy link
Collaborator

Use existing VeeValidate components like Form, Field(FormField) and ErrorMessage(FormMessage)

Other components are inspired by Shadcn

@sadeghbarati sadeghbarati marked this pull request as draft September 25, 2023 16:01
@sadeghbarati
Copy link
Collaborator Author

@ahmedmayara Hi 👋

Feel free to work on this PR, if you have time! if you have a better implementation let me know 🙌

@ahmedmayara
Copy link
Collaborator

Sure I'll check it out, I didn't have time to work on it recently.

@sadeghbarati
Copy link
Collaborator Author

@zernonia

Just followed the same structure as Shadcn source (Select the theme for the dashboard) and faced these errors in AppearanceForm.vue

1. Uncaught (in promise) DOMException: Failed to execute 'querySelector' on 'Document': '[for=1-form-item]' is not a valid selector.

2. runtime-dom.esm-bundler.js:1164 Uncaught (in promise) TypeError: Cannot set properties of null (setting '_assign')
  1. This line in radix-vue

@zernonia
Copy link
Member

@zernonia

Just followed the same structure as Shadcn source (Select the theme for the dashboard) and faced these errors in AppearanceForm.vue

1. Uncaught (in promise) DOMException: Failed to execute 'querySelector' on 'Document': '[for=1-form-item]' is not a valid selector.

2. runtime-dom.esm-bundler.js:1164 Uncaught (in promise) TypeError: Cannot set properties of null (setting '_assign')
  1. This line in radix-vue

That's the first time I'm seeing this.. could you create a minimal repro and a ticket in radix-vue. I can get to it asap

@sadeghbarati
Copy link
Collaborator Author

Hard to repro can you switch to this branch? 🙏

@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Sep 27, 2023

Problems:

  1. We have to fix this bug first in both radix-vue and shadcn-vue components [Bug]: Class name duplicated #35

    1.1 normal attributes like id, disabled, class, and so forth..., should not be in defineProps if we don't need to mutate/update them inside the component,
    types of these attributes(HTMLAttributes) is handled by default with Vue runtime.d.ts

    useAttrs is enough for these attributes

    More info here


  1. You have to expose useId within radix-vue cause vee-validate FieldContext id is not enough for our case (duplicated IDs)
    or expose any other useful utilities in radix-vue shared directory instead of duplicated code in two repository

https://github.com/radix-vue/radix-vue/blob/aacb3a29c266988529820ed4b138e2dffc3b510a/packages/radix-vue/src/shared/useEmitAsProps.ts#L5
https://github.com/radix-vue/shadcn-vue/blob/2001d1be3f308227b557c1e9ed1cba03238ffc8a/apps/www/src/lib/utils.ts#L12

@zernonia
Copy link
Member

Ahh that's brilliant!! I never thought of exposing useful utilities from radix-vue!!! I'm on it!

And yeah.. using class as props for components is not a good design.. we can create a ticket that replaces all the class props in shadcn/vue

@zernonia
Copy link
Member

@sadeghbarati Both of the ticket above had merged and released in v0.4.0. Do let me know if it helps 😁

- add new-york style
- off eslint import/first rule
- use `useId` from radix-vue
@sadeghbarati sadeghbarati marked this pull request as ready for review September 28, 2023 22:50
@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Sep 28, 2023

@zernonia @ahmedmayara please review it 🙌


<FormField v-slot="{ value }" name="language">

value slotProp types is any which Idk how to fix without type narrowing
maybe we could mention vee-validate author?

Copy link
Collaborator

@ahmedmayara ahmedmayara left a comment

Choose a reason for hiding this comment

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

Looks good to me! 😁

@sadeghbarati
Copy link
Collaborator Author

Okay 😄 let's wait for Toast component #56

@zernonia
Copy link
Member

zernonia commented Oct 1, 2023

Nicelt done @sadeghbarati ! Sorry missed out this ticket few days ago 😂
Can we quickly add the form.md?

@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Oct 1, 2023

Ok will do 🫡

What about mentioning some differences of vee-validate

  • Difference between field(native inputs) and componentField(input components) in slotProps

  • Form and useForm

  • defineInputBinds (native input elements) and defineComponentBinds (input components) defineXXXBinds will be deprecated and will replaced with defineField

  • useFieldModel deprecate❌

  • defineInputBinds deprecate❌

  • defineComponentBinds deprecate❌

  • defineField

Since my English is bad I may need help on form.md 🫠

@zernonia
Copy link
Member

zernonia commented Oct 6, 2023

The PR looks freaking good @sadeghbarati !!! The docs for form looks good for now.
In terms of the difference between different usage, we can add it in in the next PR. 😁

@zernonia zernonia merged commit d03067d into unovue:dev Oct 6, 2023
1 check failed
@zernonia zernonia linked an issue Oct 6, 2023 that may be closed by this pull request
2 tasks
@sadeghbarati sadeghbarati deleted the feat/vee-validate branch October 6, 2023 07:43
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.

[Feature]: Form Component
3 participants