-
Notifications
You must be signed in to change notification settings - Fork 544
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Live Preview ready!
|
@benjamincanac I merged this branch with #491 to fix conflicts and make it easier to test. |
No you can go ahead, it should be easy to merge |
@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 |
What do you mean by exposed attributes? |
The Form's public functions here: |
Have you tried setting |
Just tried it, it breaks everything:
|
Mmmh, it might be easier to document this manually then. |
@benjamincanac the PR is ready. It incorporates significant refactoring and simplifications, but I've made sure to maintain compatibility with previous examples. |
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'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() |
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.
Why remove the type here?
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.
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.
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.
Maybe to type the clear
method?
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(() => { |
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.
Can we just not call it error
here as it's provided this way?
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 renamed it do avoid dupe keys warning in the return statement, we can rename it to error if you prefer
@benjamincanac I applied your suggestions and I confirm that all the issues you mentioned will be solved by this PR. |
No worries at all! Thanks a lot for the changes 😊 I've updated the |
Looks good! Thanks! |
@romhml Not sure about my last commit: |
It might be better to emit the event only if the path is provided in emitFormEvent |
Can I let you improve it? 😊 |
@benjamincanac Never mind what I had in mind did not work. Your changes are good to go as is, no need for requiring the |
provide('form-group', { | ||
error, | ||
name: computed(() => props.name), | ||
size: computed(() => props.size) |
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.
@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.
Co-authored-by: Benjamin Canac <[email protected]>
This PR aims to give more control over the form component and improve input validation. It features:
validate-on
to specify when validation should happen