-
Notifications
You must be signed in to change notification settings - Fork 622
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
chore(FormGroup): simplify bindings between input and form group p⦠#704
Merged
Merged
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
3802a41
refactor(FormGroup): simplify bindings between input and form group pβ¦
romhml d4ac8ec
fix: fixed checkbox and radio default color
romhml fb2442b
fix(FormGroup): use form group name as default in input components
romhml c840133
fix(FormGroup): fixed default size for input components
romhml 8e228c8
Merge branch 'dev' of github.com:nuxt/ui into refactor-form-group-props
romhml 734a676
chore: update lockfile
romhml 5f5450d
Merge branch 'dev' into refactor-form-group-props
benjamincanac 95c719a
chore: update lockfile
romhml 3bd92d4
fix(FormGroup): set checkbox and radio ids
romhml d5de107
Merge branch 'dev' into refactor-form-group-props
benjamincanac d58d7a6
Merge branch 'dev' into refactor-form-group-props
benjamincanac ba9b62e
fix(FormGroup): fix default inputId
romhml File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<template> | ||
<div :class="ui.wrapper"> | ||
<input | ||
:id="id" | ||
:id="inputId" | ||
ref="input" | ||
:name="name" | ||
:value="modelValue" | ||
|
@@ -119,7 +119,7 @@ export default defineComponent({ | |
}, | ||
size: { | ||
type: String as PropType<keyof typeof config.size>, | ||
default: () => config.default.size, | ||
default: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @romhml Why did you move the default to the |
||
validator (value: string) { | ||
return Object.keys(config.size).includes(value) | ||
} | ||
|
@@ -154,10 +154,7 @@ export default defineComponent({ | |
setup (props, { emit, slots }) { | ||
const { ui, attrs } = useUI('ui.input', props.ui, config, { mergeWrapper: true }) | ||
|
||
const { emitFormBlur, emitFormInput, formGroup } = useFormGroup(props) | ||
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, config) | ||
|
||
const input = ref<HTMLInputElement | null>(null) | ||
|
||
|
@@ -261,7 +258,8 @@ export default defineComponent({ | |
ui, | ||
attrs, | ||
// eslint-disable-next-line vue/no-dupe-keys | ||
id, | ||
name, | ||
inputId, | ||
input, | ||
isLeading, | ||
isTrailing, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 add a
label
here? This should withUFormGroup
right?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.
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
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 but both behaviour should work, right?
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.
Yep, both are working! https://ui-git-fork-romhml-refactor-form-group-props-nuxt-js.vercel.app/dev/forms/form#input-events
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.
Does clicking on the
Checkbox
label (one fromFormGroup
) works for you? it doesn't for me.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.
Yeah sorry I could not see straight, just fixed 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.
The
label
of theRadio
components doesn't seem to work while the one from theFormGroup
does.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.
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).