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

Levy-from-Odessa · 2023-09-17T14:37:09Z

Closes #673.

vercel · 2023-09-17T14:37:12Z

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

Name	Status	Preview	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	Oct 2, 2023 5:53pm

Sma11X · 2023-09-19T06:46:24Z

src/runtime/components/forms/Range.vue

-    const size = computed(() => formGroup?.size?.value ?? props.size)
+    const size = computed(() => {
+      const size = formGroup?.size?.value ?? props.size
+      if (!ui.value.size[size.value] ||


I think we don't need to check too many, that issue happens because the range component do not support the xl size from formGroup.

agree, is it better now?

I would prefer return to props.size, that might be better for users want to define range size themselves, and it has default value already.
Return to default will overwrite the size passed by range if range not support formGroup size.

Sma11X · 2023-09-22T01:53:24Z

src/runtime/components/forms/Range.vue

@@ -89,7 +89,13 @@ export default defineComponent({

    const { emitFormChange, formGroup } = useFormGroup()
    const color = computed(() => formGroup?.error?.value ? 'red' : props.color)
-    const size = computed(() => formGroup?.size?.value ?? props.size)
+    const size = computed(() => {


#704 had conflicts with it, maybe this PR needs to take another look ...

I have pulled the dev and I returned the size.default as a fallback according to the #704 MR

Sma11X · 2023-09-22T03:00:33Z

src/runtime/components/forms/Range.vue

@@ -92,7 +92,8 @@ export default defineComponent({
  setup (props, { emit }) {
    const { ui, attrs, attrsClass } = useUI('range', props.ui, config)

-    const { emitFormChange, inputId, color, size, name } = useFormGroup(props, config)
+    const { emitFormChange, inputId, color, size: formGroupSize, name } = useFormGroup(props, config)
+    const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : config.default?.size)


Suggested change

const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : config.default?.size)

const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : (props.size ?? config.default?.size))

How about this ? I'm not sure if range component still need to custom in formGroup thou.

romhml · 2023-09-22T17:27:51Z

src/runtime/components/forms/Range.vue

-    const { emitFormChange, inputId, color, size, name } = useFormGroup(props, config)
+    const { emitFormChange, inputId, color, size: formGroupSize, name } = useFormGroup(props, config)
+    const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : (props.size ?? config.default?.size))


I recently refactored how FormGroup props are injected (#704), it might be better to do this check for all inputs by doing here: https://github.com/nuxt/ui/blob/dev/src/runtime/composables/useFormGroup.ts#L47

Also, props.size should have priority over formGroupSize

I guess something like this should work?

# useFormGroup.ts export const useFormGroup = (...) => { return { // ... size: computed(() => { const formGroupSize = config.sizes[formGroup?.size.value] ? formGroup?.size.value : undefined return inputProps?.size ?? formGroupSize ?? config?.default?.size }), // ... } }

@romhml , could you 2-check the last commit, pls?
And I think we really should add types to the ui.config.ts - file..... It would be great to use types inside useUI or useFormGroup instead of any. Am I wrong, or i can create an issue for it?

romhml · 2023-09-22T17:28:58Z

Also, wouldn't it make sense to have the same size variants for all components, or at least inputs to avoid that kind of edge cases? @benjamincanac

benjamincanac · 2023-09-22T21:24:49Z

Also, wouldn't it make sense to have the same size variants for all components, or at least inputs to avoid that kind of edge cases? @benjamincanac

Input, Textarea, Select and SelectMenu have the same sizes, it's only for Range and in a near future Toggle (#224) that have only 3 sizes. I don't think it matters that all components have the same sizes as a user could potentially define a new size on a specific component in their app.config.ts.

Sma11X · 2023-09-23T04:05:12Z

The latest commit cannot deploy

Levy-from-Odessa · 2023-09-24T16:41:52Z

The latest commit cannot deploy
could you describe what is the issue here, pls?

Sma11X · 2023-09-25T01:43:29Z

Try the same code as issue at playground and will return 500. It can't read xl now. No need to use ui instead of config.

src/runtime/composables/useFormGroup.ts

Co-authored-by: Sma11X <540351143@qq.com>

Sma11X · 2023-10-01T04:34:59Z

src/runtime/components/forms/Range.vue

@@ -92,7 +92,7 @@ export default defineComponent({
  setup (props, { emit }) {
    const { ui, attrs, attrsClass } = useUI('range', props.ui, config)

-    const { emitFormChange, inputId, color, size, name } = useFormGroup(props, config)
+    const { emitFormChange, inputId, color, size, name } = useFormGroup(props, ui)


Suggested change

const { emitFormChange, inputId, color, size, name } = useFormGroup(props, ui)

const { emitFormChange, inputId, color, size, name } = useFormGroup(props, config)

src/runtime/composables/useFormGroup.ts

benjamincanac · 2023-10-27T14:38:09Z

@romhml I've followed your advice in the end and added sizes to the Range component in 3cb3914 so it fixes #673.

Does this PR is still relevant in your opinion? It might prevent bugs in the future I guess.

romhml · 2023-10-28T11:38:57Z

@benjamincanac I agree, it's still good idea to fallback to the default input's size if the one provided by the FormGroup is not defined, to handle cases where a user defines custom sizes.

fix(Range): add default size

b1455db

vercel bot deployed to Preview September 17, 2023 14:41 View deployment

Sma11X reviewed Sep 19, 2023

View reviewed changes

fix(Range): fix default size

d0afcd9

vercel bot deployed to Preview September 19, 2023 17:07 View deployment

fix(Range): set default size

c3c87d7

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

SCHrodrigo mentioned this pull request Sep 21, 2023

feat(FormGroup): add slots #714

Merged

8 tasks

Sma11X reviewed Sep 22, 2023

View reviewed changes

fix(Range): pull dev

aa0c1f8

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

Sma11X reviewed Sep 22, 2023

View reviewed changes

benjamincanac requested a review from romhml September 22, 2023 08:20

fix(Range): defaul size

f427c8c

vercel bot deployed to Preview September 22, 2023 15:30 View deployment

romhml reviewed Sep 22, 2023

View reviewed changes

saveliy added 2 commits September 22, 2023 15:58

Merge branch 'dev' of https://github.com/nuxt/ui into dev

bc90fb9

fix(Range): adjust useFormGroup size var

2a6b4f7

vercel bot had a problem deploying to Preview September 22, 2023 23:21 Failure

Sma11X reviewed Sep 25, 2023

View reviewed changes

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

Update src/runtime/composables/useFormGroup.ts

a66e345

Co-authored-by: Sma11X <540351143@qq.com>

vercel bot had a problem deploying to Preview September 28, 2023 21:37 Failure

Sma11X suggested changes Oct 1, 2023

View reviewed changes

fix(Range): default size

aafb767

vercel bot deployed to Preview October 2, 2023 17:53 View deployment

benjamincanac requested a review from romhml October 26, 2023 15:13

romhml approved these changes Oct 28, 2023

View reviewed changes

benjamincanac approved these changes Oct 28, 2023

View reviewed changes

benjamincanac changed the title ~~fix(Range): add default size~~ fix(FormGroup): ensure size exists in config Oct 28, 2023

benjamincanac merged commit f5f3388 into nuxt:dev Oct 28, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Levy-from-Odessa commented Sep 17, 2023

vercel bot commented Sep 17, 2023 •

edited

Loading

Sma11X Sep 19, 2023 •

edited

Loading

Levy-from-Odessa Sep 19, 2023

Sma11X Sep 20, 2023

Levy-from-Odessa Sep 21, 2023

Sma11X Sep 22, 2023

Levy-from-Odessa Sep 22, 2023

Sma11X Sep 22, 2023

romhml Sep 22, 2023

Levy-from-Odessa Sep 22, 2023

romhml commented Sep 22, 2023

benjamincanac commented Sep 22, 2023

Sma11X commented Sep 23, 2023

Levy-from-Odessa commented Sep 24, 2023

Sma11X commented Sep 25, 2023

Sma11X Oct 1, 2023

benjamincanac commented Oct 27, 2023

romhml commented Oct 28, 2023

	const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : config.default?.size)
	const size = computed(() => ui.value.size[formGroupSize] ? formGroupSize : (props.size ?? config.default?.size))

	const { emitFormChange, inputId, color, size, name } = useFormGroup(props, ui)
	const { emitFormChange, inputId, color, size, name } = useFormGroup(props, config)

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

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

Conversation

Levy-from-Odessa commented Sep 17, 2023

vercel bot commented Sep 17, 2023 • edited Loading

Sma11X Sep 19, 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

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 22, 2023

benjamincanac commented Sep 22, 2023

Sma11X commented Sep 23, 2023

Levy-from-Odessa commented Sep 24, 2023

Sma11X commented Sep 25, 2023

Choose a reason for hiding this comment

benjamincanac commented Oct 27, 2023

romhml commented Oct 28, 2023

vercel bot commented Sep 17, 2023 •

edited

Loading

Sma11X Sep 19, 2023 •

edited

Loading