-
Notifications
You must be signed in to change notification settings - Fork 458
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
fix(FormGroup): Use explicit label instead of implicit label #638
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This results in a unique constraint on FormGroup name attributes within a page which is not ideal. I found another way to fix the issue (https://github.com/nuxt/ui/pull/651/files) by preventing the click event from propagating to the label which caused the menu to open. |
I apologize for not mentioning the reason earlier. The use of implicit labels in FormGroup restricts it to conforming with the permitted content that labels can have, as mentioned in this link. Consequently, FormGroup can only be used for Input, Textarea, and Select elements. I believe it's acceptable to have constraints on the FormGroup name attribute since we also utilize it as a path in the Form. |
For what form elements would you use the |
Sorry, I made a typo in 'Consequently, FormGroup can only be used for Input, Textarea, and Select elements.' What I meant was Input, Textarea, and Select custom components. Yes, you are correct; it can be used for 'input,' 'textarea,' or 'select.' However, for complex custom components like |
Hi @benjamincanac and @romhml , I tried adding an autogenerated ID assigned to the label attribute. What do you think? |
@aditio-eka I experimented using FormGroup's with complex input component, and did not experience any problem (except for interactivity issues, that can be fixed by stopping the event propagation). Here is an example with a date-picker: https://stackblitz.com/edit/nuxt-ui-rpmghm?file=app.vue Do you have any example where the current implementation does not work as expected so I can look into it? |
@benjamincanac can we move forward with #651 to fix interactivity issues with SelectMenu and other components? |
Closing this in favor of #651 |
hai @benjamincanac and @romhml, Until now, I have also only encountered interactivity issues, but those have been addressed in my recent pull request (PR). I believe these issues arise due to the constraints of implicit labeling:
My recent PR aims to resolve these issues, and so far, it has been effective in preventing the problems you mentioned. Can we open the PR again? I think it doesn't sound ideal if we always have to add a stop propagation on the click event when creating new complex input components. References: |
@aditio-eka you're right, I did not notice it, but #651 introduces other issues with checkbox and radio components inside FormGroups (FYI @benjamincanac). Removing nested labels might be the only solution to properly address the issue. I checked NaiveUI's implementation for reference but it looks like they don't bind label and inputs at all (https://github.com/tusen-ai/naive-ui/blob/main/src/form/src/FormItem.tsx#L84). We're left with using |
@@ -93,13 +97,17 @@ export default defineComponent({ | |||
|
|||
const size = computed(() => ui.value.size[props.size ?? appConfig.ui.input.default.size]) | |||
|
|||
const labelFor = `label-for-${increment = increment < 1000000 ? increment + 1 : 0}` |
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.
What about something like this to generate the random id?
const seed = Math.random().toString(36).substring(7)
const labelFor = `${props.name}-${seed}`
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.
const seed = Math.random().toString(36).substring(7)
can cause mismatched hydration as the server and client can have different values.
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, this is my reference for why I use an increment instead of a random value.
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.
Wouldn't it be better to add the prop.name
inside the labelFor
? ${props.name}-${increment...}
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.
Wouldn't it be better to add the
prop.name
inside thelabelFor
?${props.name}-${increment...}
okay
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.
@benjamincanac @romhml updated
Maybe components like repeater style where you could have multiple |
@romhml Should we reopen this PR and revert your changes then? |
@benjamincanac Yes! |
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.
We should apply the same behaviour on Radio
and Checkbox
components so these can be used with the FormGroup
too:
<input
:id="labelFor || `${name}-${value}`"
@aditio-eka Would you mind fixing the conflicts and at the same time reverting the |
@benjamincanac, We should not apply it to
Yes, I would |
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.
Looks good @aditio-eka @benjamincanac! I added just one suggestion to avoid overriding existing the id
attribute if it's already provided by the user (which may be unexpected from the user's point of view).
import type { FormEvent, FormEventType, InjectedFormGroupValue } from '../types/form' | ||
|
||
export const useFormGroup = () => { | ||
const formBus = inject<UseEventBusReturn<FormEvent, string> | undefined>('form-events', undefined) | ||
const formGroup = inject('form-group', undefined) | ||
const formGroup = inject<InjectedFormGroupValue>('form-group') | ||
|
||
const blurred = ref(false) | ||
|
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 think we can tweak this composable to avoid overriding the id
attribute if it's explicitly provided by the user like so:
<FormGroup name="foo">
<UInput id="bar" /> <!-- will be replaced by 'foo-1' -->
</FormGroup>
It can be achieved with something like this:
export const useFormGroup = (inputAttrs: { id: string }) => {
const formBus = inject<UseEventBusReturn<FormEvent, string> | undefined>('form-events', undefined)
const formGroup = inject<InjectedFormGroupValue>('form-group', undefined)
if (formGroup) {
// Update labelFor if input already has an id
formGroup.labelFor.value = inputAttrs?.id ?? formGroup?.labelFor.value
}
// [...]
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 a good suggestion @romhml . If we want to avoid overriding the id
attribute, I think we should also avoid overriding the name
and size
attributes. In the current source code, the name
and size
attributes can be overridden by UFormGroup
. It would be good to have the same behavior for attribute assignment.
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.
Agreed, this needs some cleanup to make it consistent across all properties, looks like:
size
gives priority to the form group's valuename
does not seem used outside of the Checkbox component and can be removed from the input props (not directly related but clicking on a Checkbox's label is broken if no name attribute is provided)
Ideally the priority should be set on the input's value and use the FormGroup's if none is provided.
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'd suggest to handle the id
properly in this PR and open a new one to implement a consistent behaviour for other props to keep things manageable, what do you think?
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.
Yes, that's great, @romhml. I will handle 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.
@romhml updated
const formBus = inject<UseEventBusReturn<FormEvent, string> | undefined>('form-events', undefined) | ||
const formGroup = inject('form-group', undefined) | ||
const formGroup = inject<InjectedFormGroupValue>('form-group') |
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.
const formGroup = inject<InjectedFormGroupValue>('form-group') | |
const formGroup = inject<InjectedFormGroupValue>('form-group', undefined) |
Fixes [Vue warn]: injection "form-group" not found.
@@ -92,14 +95,17 @@ export default defineComponent({ | |||
}) | |||
|
|||
const size = computed(() => ui.value.size[props.size ?? appConfig.ui.input.default.size]) | |||
const labelFor = computed(() => `${props.name}-${increment = increment < 1000000 ? increment + 1 : 0}`) |
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.
const labelFor = computed(() => `${props.name}-${increment = increment < 1000000 ? increment + 1 : 0}`) | |
const labelFor = ref(`${props.name}-${increment = increment < 1000000 ? increment + 1 : 0}`) |
This must be changed to a ref to allow write operations in useFormGroup
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.
Hi @romhml, I've changed how to update labelFor
when the input already has an id
.
from
formGroup.labelFor.value = inputAttrs?.id ?? formGroup?.labelFor.value
to
const labelFor = computed(() => inputAttrs?.id || formGroup?.labelFor.value)
It will ensure that our labelFor
value is updated based on the new id
attribute of UInput
and the new name
attribute of UFormGroup
.
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'm not able to test it right now but I think this won't update the for=
attribute in the FormGroup if an id is provided
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, these are my testing results. I hope they can be helpful.
Screen.Recording.2023-09-19.at.16.11.52.mov
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.
Here's an example of what I meant:
<UFormGroup label="Email" name="email">
<UInput id="foo" v-model="state.email" />
</UFormGroup>
Results in the following:
<label for="email-4" ...>Email</label>
<input id="foo" type="text" ...>
Instead of:
<label for="foo" ...>Email</label>
<input id="foo" type="text" ...>
Which breaks accessibility features and interactions (like clicking on the label to focus the input).
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.
Using
const labelFor = ref(`${props.name}-${increment = increment < 1000000 ? increment + 1 : 0}`)
allows to properly update the value in useFormGroup
to handle explicit ids properly. Reactivity can then be achieved using watch
, although I don't think it's needed here (can't see a lot of use cases)
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.
Sorry, @romhml, I missed the previous comment. I have updated it.
... although I don't think it's needed here (can't see a lot of use cases)
I assumed there was a bug when there is a field array, but that's not the case. I tested it with my new update, and the bug doesn't appear.
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.
Looks good to me! Thanks a lot @aditio-eka!
You're welcome, and thanks for your review as well! |
Thanks guys for you work! 😊 |
Fixes #621, fixes #675