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

NumberInput component improvements #970

Closed
pavish opened this issue Jan 11, 2022 · 2 comments
Closed

NumberInput component improvements #970

pavish opened this issue Jan 11, 2022 · 2 comments
Assignees
Labels
type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory

Comments

@pavish
Copy link
Member

pavish commented Jan 11, 2022

Problem

The NumberInput component implemented as part of #915 is a low level component with few practical uses. The basic version was implemented inorder to support the form-builder component. Inorder to make it usable, it requires a number of improvements.

Proposed solution

The following contents are taken from @seancolsen's comments on the form-builder PR

Fix NumberInput setting value to string

The two-way binding for value should reliably produce a type of number | undefined.

Case 1

  1. Instantiate a NumberInput component with a bound value equal to 7.
  2. In the browser focus on the input, placing the cursor at the end.
  3. Enter 8.
  4. Expect value to be 78 (number).
  5. Observe value to be "78" (string).

Case 2

  1. Instantiate a NumberInput component with a bound value equal to 7.
  2. In the browser focus on the input, placing the cursor at the end.
  3. Press Backspace once, clearing the contents of the input.
  4. Expect value to be undefined.
  5. Observe value to be "" (empty string).

Furthermore (and on a related note), I'm a bit confused about what's happening with interface HTMLNumberInputElement, and my hunch is that we should remove it.

My understanding is that the value property of an <input> will always be a string, even when setting type='number'. That means that writing e.target as HTMLNumberInputElement (in NumberInput.svelte) is incorrectly providing type assurance that value will be a number.

I would expect NumberInput to assume that the underlying DOM element will have a string value, and to be converting that string to a number (i.e. with Number(inputElement.value)) to produce a binding to the value prop that satisfies number | undefined.

More gracefully handle some edge cases in NumberInput

Independent sub-issues, with steps to reproduce.

  • Support pasting multi-character values

    1. Begin with a value of 1
    2. Into your clipboard, copy 22
    3. Focus on the input and select all its contents
    4. Paste
    5. Expect 22
    6. Observe 1

    Adding a + after the group in validKeyRegex appear to fix this.

  • Support entering -0.1

    1. Begin with an empty input
    2. Enter -0 and observe -0 which is good
    3. Type .
    4. Expect -0.
    5. Observe 0
  • Allow empty integer

    1. Begin with 1 and isInteger
    2. Place cursor at end
    3. Press Backspace once
    4. Expect empty input
    5. Observe 0
  • Don't wipe contents when entering 7..

    1. Begin with an empty input
    2. Enter 7. and observe 7. which is good
    3. Type .
    4. Expect 7.
    5. Observe an empty input
  • Retain cursor position

    1. Begin with 78
    2. Move cursor between 7 and 8
    3. Enter a
    4. Expect 78 with cursor between 7 and 8
    5. Observe 78 with cursor after 8
  • Don't auto-delete decimal

    1. Begin with 1.2
    2. Delete the 2
    3. Expect 1.
    4. Observe 1
  • Don't auto-delete minus sign

    1. Begin with -8
    2. Delete the 8
    3. Expect -
    4. Observe an empty input
  • Don't truncate integer value when entering decimal midway through number

    1. Begin with 23 and isInteger
    2. Place cursor between 2 and 3
    3. Enter .
    4. Expect 23
    5. Observe 2
  • Remove support for numbers like 8e9, or allow users to enter them. I can enter 89, then place my cursor between the two digits and type e. But I can't enter 8e9 directly. I don't think we need support entering these kinds of numbers into the input though.

Additional thoughts

  • I'd also like to stress test this a bit more after the string type issue is fixed.
  • It might be worth using the beforeinput event instead of the input event.

Add tests for NumberInput

This is the kind of component that really benefits from having some tests in my opinion.

We could pretty easily write tests for the scenarios I've laid out above, as well as other ones that already work correctly.

Remove type='number' on NumberInput

I hate the browser native number input, and I think most people do too.

  • I don't like the tiny up/down buttons. They look bad and don't add much value.
  • The value of the input changes in response to scroll events, which can be really annoying sometimes.

I'd prefer that we remove

Considerations:

  • We'll need to set inputmode to either decimal or numeric, potentially with some logic based on the props.
  • We'll need to adjust some of the logic within the onInput function to account for the different behavior of inputElement.value when the user enters a non-numeric string like x into the input.
@pavish pavish added type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory needs: unblocking Blocked by other work labels Jan 11, 2022
@pavish
Copy link
Member Author

pavish commented Jan 11, 2022

Blocked by #915

@pavish pavish added this to the [07.2] 2022-02 improvements milestone Jan 11, 2022
@pavish pavish mentioned this issue Jan 11, 2022
7 tasks
@kgodey kgodey added ready Ready for implementation and removed needs: unblocking Blocked by other work labels Jan 28, 2022
@seancolsen seancolsen self-assigned this Mar 3, 2022
@seancolsen seancolsen added status: started and removed ready Ready for implementation labels Mar 8, 2022
@pavish
Copy link
Member Author

pavish commented Mar 28, 2022

Handled in #1155

@pavish pavish closed this as completed Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

No branches or pull requests

3 participants