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.

Sign up for GitHub

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(VNumberInput): clearing v-number-input with backspace resets to 0 #20729

Merged
merged 3 commits into from
Dec 14, 2024
Merged

fix(VNumberInput): clearing v-number-input with backspace resets to 0 #20729

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55208ae
fix(VNumberInput): expect empty string on reset to perform validation
EvgenyWas Nov 24, 2024
3c18a5e
Merge branch 'master' into fix/20607-clearing-v-number-input-with-bac…
EvgenyWas Nov 30, 2024
6a55990
refactor: keep _model.value as null when assigning empty string
yuwu9145 Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions packages/vuetify/src/labs/VNumberInput/VNumberInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,14 @@ export const VNumberInput = genericComponent<VNumberInputSlots>()({
const model = computed({
get: () => _model.value,
set (val) {
if (val === null) {
_model.value = null
if (val === null || (typeof val === 'string' && !val)) {
J-Sek marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Clearing number via backspace currently results in warning:

[Vue warn]: Invalid prop: type check failed for prop "modelValue". Expected Number with value 0, got String with value "". 
  at <VNumberInput ref="input" model-value="" rules= (3) [ƒ, ƒ, ƒ]  ... > 
  at <VContainer> 
  at <VApp> 
  at <Playground> 
  at <App>

modelValue here should be strictly enforced with type number | null, probably try to set _model.value to null when backspace to empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @yuwu9145, thanks for the review!

As I described in the PR description - the useValidation composable doesn't perform validation when the input value changes to null. When resetting the input, it sets the value to an empty string. Instead of assigning 0, the model should be reset to match this behavior. Using null for resetting is not viable because validation won't be triggered. Therefore, the model value should remain as the empty string in such cases to ensure proper validation behavior.

I think it might be worth of refactoring to extend the model type with string. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could put || val === '' here (PR #20252) and it would just work without any issues with TS nor warnings.

Copy link
Member

Choose a reason for hiding this comment

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

I still believe model value should be set to null in number input.

As for the issue of null doesn't trigger validation, it's a separate issue that should be resolved in validation composable demo

Copy link
Member

@yuwu9145 yuwu9145 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#20765

This PR should aim to make number input work consistently with text field in terms of validation, but model value type is kept with number | null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens because of this condition in useValidation composable, but it was added over two years ago and I assume it can affect many places if we change it.

Copy link
Member

@yuwu9145 yuwu9145 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's not a concern in VNumberInput and validation issue should be carefully resolved/tested in #20765

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we can close this PR or just solve the first part of the issue regarding resetting the value?

Copy link
Member

Choose a reason for hiding this comment

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

Just need to resolve the issue regarding resetting the value to null, and ignore validation.

I have pushed a commit, see if you had any comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yuwu9145 it's fine for me, thanks for your participation and prompt reply! :)

_model.value = val
return
}

if (!isNaN(+val) && +val <= props.max && +val >= props.min) {
_model.value = +val
const value = Number(val)
if (!isNaN(value) && value <= props.max && value >= props.min) {
_model.value = value
}
},
})
Expand Down Expand Up @@ -294,9 +295,9 @@ export const VNumberInput = genericComponent<VNumberInputSlots>()({

{ incrementControlNode() }
</div>
) : (!props.reverse
Copy link
Member

@yuwu9145 yuwu9145 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change just a code format? Or was there any logically concrete reason behind it? @EvgenyWas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only code format to remove the exclamation mark. I believe it's easier to read especially when there is the same condition with another case.

? <>{ dividerNode() }{ controlNode() }</>
: undefined)
) : (props.reverse
? undefined
: <>{ dividerNode() }{ controlNode() }</>)

const hasAppendInner = slots['append-inner'] || appendInnerControl

Expand Down