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

refactor(FormGroup): use inject instead of cloning slots #491

Closed
wants to merge 4 commits into from
Closed

refactor(FormGroup): use inject instead of cloning slots #491

wants to merge 4 commits into from

Conversation

romhml
Copy link
Collaborator

@romhml romhml commented Aug 3, 2023

This is a proposal to refactor the FormGroup component. It replaces vProps / slot manipulations by injecting relevant attributes directly into the children components.

This allows the use of v-slots (fixes #484) and provides an easy way to make custom input components compatible with the Form / FormGroup components using useFormGroup.

I also replaced how labels and inputs are bound by nesting them directly, to avoid conflicts with duplicate ids c.f.

Alternatively, you can nest the input directly inside the label, in which case the for and id attributes are not needed because the association is implicit:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#attributes

@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 3, 2023 6:44pm

Comment on lines +30 to +33
name: {
type: String,
default: null
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can depreciate this props since it's now only used for validation and rename it to path to make the documentation clearer.

The two can coexist by setting the validation path to props.path ?? props.name to avoid breaking changes

Copy link
Member

Choose a reason for hiding this comment

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

You still need it to bind on the <label :for="name">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not if the input is wrapped into the label like I mentioned in the description

Copy link
Member

Choose a reason for hiding this comment

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

Completely missed that sorry, it's awesome! Nice found 😊

@benjamincanac
Copy link
Member

Just made this commit 83c42cd that fixes the size on the FormGroup.

Do you need me to solve the conflicts?

@romhml
Copy link
Collaborator Author

romhml commented Aug 3, 2023

I'll do it if you're good with the changes, I need adapt the other input components too

@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 4751aa9

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.

[Documentation] FormGroup error should not display the trailing icon when there is no error.
2 participants