chore(FormGroup): simplify bindings between input and form group p… #704

romhml · 2023-09-20T18:21:59Z

🔗 Linked issue

❓ Type of change

📖 Documentation (updates to the documentation or readme)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR refactors the way the FormGroup component injects props into input components. It also introduces a util function to generate unique unique html identifiers (fixes #680).

In short:

Inputs that include a label (Checkbox and Radio) now uses a random id instead of the name attribute to avoid conflicts
An input's prop will always have priority over the value that is provided by the FormGroup (c.f. fix(FormGroup): Use explicit label instead of implicit label #638 (comment))

📝 Checklist

I have linked an issue or discussion.
I have updated the documentation accordingly.

vercel · 2023-09-20T18:22:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	Sep 21, 2023 4:16pm

romhml · 2023-09-20T18:27:37Z

FYI @benjamincanac @aditio-eka

…rops

benjamincanac · 2023-09-20T20:05:44Z

src/runtime/components/forms/Input.vue

-    const color = computed(() => formGroup?.error?.value ? 'red' : props.color)
-    const size = computed(() => formGroup?.size?.value ?? props.size)
-    const id = formGroup?.labelFor
+    const { emitFormBlur, emitFormInput, size, color, inputId, name } = useFormGroup(props)


Suggested change

const { emitFormBlur, emitFormInput, size, color, inputId, name } = useFormGroup(props)

const { emitFormBlur, emitFormInput, size, color, inputId } = useFormGroup(props)

Why is the name not used to bind on the <input>?

I went too fast, fixed it here fb2442b

benjamincanac · 2023-09-20T20:12:57Z

src/runtime/composables/useFormGroup.ts

@@ -38,9 +46,12 @@ export const useFormGroup = (inputAttrs?: InputAttrs) => {
    }, 300)

    return {
+      inputId: formGroup?.inputId,
+      name: computed(() => inputProps?.name ?? formGroup?.name),
+      size: computed(() => inputProps?.size ?? formGroup?.size?.value ?? appConfig.ui.input.default.size),


Unfortunately appConfig.ui.input.default.size won't work here as the default config no longer lives in the appConfig, it's only the one from the user. You can take a look at the FormGroup: https://github.com/nuxt/ui/pull/704/files#diff-82ffaa8fe9c377bdfd6259594303b0111befc57be8db4b95e1e1d4cd0d3eb56dR30

import { formGroup } from '#ui/ui.config'

Good catch! I looked into it, looks like it's going to be a bit more complicated than I imagined, the default value needs to be set to the input's default (not the form group), it might make more sense move this back into each components

Or you can take the default from the input as most of the other form elements inherit from its config. This could lead to an edge-case if the user changes the config entirely but I think its good enough.

I fixed it by providing the configuration object in the composable to avoid edge-cases, what do you think?

src/runtime/composables/useFormGroup.ts

pnpm-lock.yaml

benjamincanac · 2023-09-21T12:32:17Z

docs/components/content/examples/FormExampleElements.vue

@@ -80,7 +80,7 @@ async function submit (event: FormSubmitEvent<Schema>) {
    </UFormGroup>

    <UFormGroup name="checkbox" label="Checkbox">
-      <UCheckbox v-model="state.checkbox" />
+      <UCheckbox v-model="state.checkbox" label="Check me" />


Why add a label here? This should with UFormGroup right?

Checkbox components and Radio are a bit different from the others because a label is included in the component:

https://github.com/nuxt/ui/pull/704/files#diff-eeb89ce166ecb860614d9cb79ad8b64476eaf9a919c31dfccb89c50052c2c728R21

Yes but both behaviour should work, right?

Yep, both are working! https://ui-git-fork-romhml-refactor-form-group-props-nuxt-js.vercel.app/dev/forms/form#input-events

Does clicking on the Checkbox label (one from FormGroup) works for you? it doesn't for me.

Yeah sorry I could not see straight, just fixed it!

The label of the Radio components doesn't seem to work while the one from the FormGroup does.

Fixed, note that clicking on the FormGroup's label for Radio groups does not work properly, I'll fix this in another PR introducing a wrapper component (will also fix #684).

benjamincanac · 2023-09-22T08:19:12Z

src/runtime/components/forms/Input.vue

@@ -119,7 +119,7 @@ export default defineComponent({
    },
    size: {
      type: String as PropType<keyof typeof config.size>,
-      default: () => config.default.size,
+      default: null,


@romhml Why did you move the default to the useFormGroup composable?

vercel bot deployed to Preview September 20, 2023 18:26 View deployment

refactor(FormGroup): simplify bindings between input and form group p…

3802a41

…rops

vercel bot deployed to Preview September 20, 2023 18:33 View deployment

benjamincanac reviewed Sep 20, 2023

View reviewed changes

src/runtime/composables/useFormGroup.ts Show resolved Hide resolved

fix: fixed checkbox and radio default color

d4ac8ec

vercel bot deployed to Preview September 20, 2023 21:10 View deployment

fix(FormGroup): use form group name as default in input components

fb2442b

vercel bot deployed to Preview September 20, 2023 21:46 View deployment

fix(FormGroup): fixed default size for input components

c840133

vercel bot deployed to Preview September 21, 2023 12:11 View deployment

romhml added 2 commits September 21, 2023 14:14

Merge branch 'dev' of github.com:nuxt/ui into refactor-form-group-props

8e228c8

chore: update lockfile

734a676

vercel bot deployed to Preview September 21, 2023 12:24 View deployment

Merge branch 'dev' into refactor-form-group-props

5f5450d

benjamincanac reviewed Sep 21, 2023

View reviewed changes

pnpm-lock.yaml Outdated Show resolved Hide resolved

benjamincanac reviewed Sep 21, 2023

View reviewed changes

vercel bot deployed to Preview September 21, 2023 12:38 View deployment

chore: update lockfile

95c719a

vercel bot deployed to Preview September 21, 2023 12:46 View deployment

fix(FormGroup): set checkbox and radio ids

3bd92d4

vercel bot had a problem deploying to Preview September 21, 2023 13:15 Failure

Merge branch 'dev' into refactor-form-group-props

d5de107

vercel bot deployed to Preview September 21, 2023 13:22 View deployment

Merge branch 'dev' into refactor-form-group-props

d58d7a6

vercel bot deployed to Preview September 21, 2023 14:09 View deployment

fix(FormGroup): fix default inputId

ba9b62e

vercel bot deployed to Preview September 21, 2023 16:16 View deployment

benjamincanac approved these changes Sep 21, 2023

View reviewed changes

benjamincanac changed the title ~~refactor(FormGroup): simplify bindings between input and form group p…~~ chore(FormGroup): simplify bindings between input and form group p… Sep 21, 2023

benjamincanac merged commit 46879dc into nuxt:dev Sep 21, 2023
1 check passed

Sma11X mentioned this pull request Sep 22, 2023

fix(FormGroup): ensure size exists in config #695

Merged

benjamincanac reviewed Sep 22, 2023

View reviewed changes

benjamincanac mentioned this pull request Dec 4, 2023

[Forms] Lost size defaults #1059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(FormGroup): simplify bindings between input and form group p… #704

chore(FormGroup): simplify bindings between input and form group p… #704

romhml commented Sep 20, 2023 •

edited

Loading

vercel bot commented Sep 20, 2023 •

edited

Loading

romhml commented Sep 20, 2023

benjamincanac Sep 20, 2023

benjamincanac Sep 20, 2023

romhml Sep 21, 2023

benjamincanac Sep 20, 2023

romhml Sep 20, 2023

benjamincanac Sep 21, 2023

romhml Sep 21, 2023

benjamincanac Sep 21, 2023

romhml Sep 21, 2023

benjamincanac Sep 21, 2023

romhml Sep 21, 2023

benjamincanac Sep 21, 2023 •

edited

Loading

romhml Sep 21, 2023

benjamincanac Sep 21, 2023

romhml Sep 21, 2023

benjamincanac Sep 22, 2023

	const { emitFormBlur, emitFormInput, size, color, inputId, name } = useFormGroup(props)
	const { emitFormBlur, emitFormInput, size, color, inputId } = useFormGroup(props)

chore(FormGroup): simplify bindings between input and form group p… #704

chore(FormGroup): simplify bindings between input and form group p… #704

Conversation

romhml commented Sep 20, 2023 • edited Loading

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

vercel bot commented Sep 20, 2023 • edited Loading

romhml commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincanac Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romhml commented Sep 20, 2023 •

edited

Loading

vercel bot commented Sep 20, 2023 •

edited

Loading

benjamincanac Sep 21, 2023 •

edited

Loading