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.

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

Conversation

EvgenyWas
Copy link
Contributor

@EvgenyWas EvgenyWas commented Nov 24, 2024

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.
fixes #20607

Markup:

<template>
  <v-app>
    <v-container>
      <v-number-input
        ref="input"
        :model-value="model"
        :rules="rules"
        validate-on="input"
        @update:model-value="onChange"
      />
      <br />
      <pre>{{ inputToJSON }}</pre>
    </v-container>
  </v-app>
</template>

<script setup>
  import { ref, computed } from 'vue'

  const input = ref()

  const model = ref()

  const rules = ref([
    // required validation
    value => {
      console.log('required', value)
      return (
        (value !== null && typeof value !== 'undefined' && value !== '') ||
        'required'
      )
    },
    // min validation
    value => {
      return value >= 5 || 'should be greater or equal 5'
    },
    // max validation
    value => {
      return value <= 10 || 'should be lower or equal 10'
    },
  ])

  const onChange = value => {
    console.log('onChange', value)
    model.value = value
  }

  const inputToJSON = computed(() => {
    return JSON.stringify(input.value, null, 2)
  })
</script>

@EvgenyWas EvgenyWas marked this pull request as ready for review November 24, 2024 14:44
@EvgenyWas EvgenyWas changed the title fix(VNumberInput): expect empty string on reset to perform validation fix(VNumberInput): clearing v-number-input with backspace resets to 0 Nov 24, 2024
J-Sek
J-Sek previously approved these changes Nov 30, 2024
@J-Sek J-Sek requested a review from yuwu9145 November 30, 2024 14:28
@@ -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)) {
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

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

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! :)

@@ -294,9 +297,9 @@ export const VNumberInput = genericComponent<VNumberInputSlots>()({

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

@yuwu9145 yuwu9145 Dec 12, 2024

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.

@yuwu9145 yuwu9145 added T: bug Functionality that does not work as intended/expected labs C: VNumberInput labels Dec 14, 2024
@yuwu9145 yuwu9145 added this to the v3.7.x milestone Dec 14, 2024
@yuwu9145 yuwu9145 merged commit 96fca07 into vuetifyjs:master Dec 14, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VNumberInput labs T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report][3.7.3] clearing v-number-input with backspace resets to 0
3 participants