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

Number component #1155

Merged
merged 27 commits into from
Mar 28, 2022
Merged

Number component #1155

merged 27 commits into from
Mar 28, 2022

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Mar 10, 2022

Summary

  • This PR alters the behavior or our NumberInput component, making it much more robust.

  • Example changes in behavior to NumberInput

    Before After
    Value produced when user enters e009..e "e009..e" (string) 9 (number)
    Formatted display when user enters e009..e e009..e 9.
    Formatted display of number 1234 1234 1,234
  • Within Mathesar, NumberInput is currently used only for the form that configures the DB settings on a text field. But we eventually want to it for editing cells, and potentially other places too.

Additional context

Demo

Peek 2022-03-21 11-52

Review tips

  • Play with the NumberInput story. In particular, try fiddling with allowFloat, allowNegative and locale to see how the behavior of the input changes.
  • Play with the input that sets the field size limit of a text column. (From what I can tell, that's the only place within Mathesar that's affected by this PR.)

Requirements from #970

  • Fix NumberInput setting value to string
  • Support pasting multi-character values
  • Support entering -0.1
  • Allow empty integer
  • Don't wipe contents when entering 7..
  • Retain cursor position -- to be addressed via new ticket
  • Don't auto-delete decimal
  • Don't auto-delete minus sign
  • Don't truncate integer value when entering decimal midway through number
  • Remove support for numbers like 8e9
  • More stress testing (I monkeyed around with it quite a bit)
  • use the beforeinput event
  • Add tests
  • Remove type='number' on NumberInput -- (already done in Text type updates #1144)
  • Set inputmode to either decimal or numeric

Additional work completed

  • Make a generalized FormattedInput component that can be composed to build other kinds of specialized inputs.

  • Allow input of numbers in other locales.

  • Use NumberInput for number numbers and StringifiedNumberInput for high precision numbers (stored as string).

  • Disallow user input into NumberInput that will result in loss of precision.

    You cay try this out by pressing 9 repeatedly on the input. You'll notice it stops accepting input after fifteen digits. In any situation where we're using NumberInput, hitting the precision limit should be a very unlikely scenario. That said, an alternate behavior (that might be nice to add at some point) would be to round the input value to the nearest storable value (instead of preventing input). This would mean you could keep typing 9 and when you get to sixteen digits, the value of the input would change to 10000000000000000. I could open a ticket for this, but I don't really think it's worth it. If someone needs this later we could address it. It wouldn't be too hard to add a prop on NumberInput to configure this behavior.

  • Set allowFloat and allowNegative to false by default. (I figure better to be strict by default. We can discuss if necessary.)

To handle later via tickets

Here are the templates to create the tickets

Support scientific notation

cat | gh issue create -F - -t 'Add support for scientific notation in number input components' -m '[Beta] Better Editing Experience' -l "status: ready" -l "type: enhancement" -l "work: frontend"

## Current behavior

- Entering numbers like `8E9` into a `NumberInput` (or `StringifiedNumberInput`) is not possible.

## Desired behavior

- `NumberInput` and `StringifiedNumberInput` accept a prop `allowScientificNotation` which defaults to `false`. When `true` the user can enter numbers in scientific notation and the component will display numbers in scientific notation when appropriate.

## Additional context

- See [Number display and entry](https://wiki.mathesar.org/en/engineering/specs/numbers) for more information about how scientific notation should work.

## Implementation

- Begin by adding `allowScientificNotation` to `Options` within `src/component-library/number-input/number-formatter/options.ts`. This change will allow the components to accept that prop.
- Many of the functions in `cleaners.ts` will need to be modified to work with scientific notation. Make sure to add tests in `cleaners.test.ts`.
- `formatter.ts` will also need modification to display numbers in scientific notation.

Retain cursor position

cat | gh issue create -F - -t 'Retain cursor position when changing text within FormattedInput' -m '[Beta] Better Editing Experience' -l "status: ready" -l "type: bug" -l "work: frontend"

## Current behavior

1. In Storybook, open the NumberInput story.
1. Set the input to contain text `134`.
1. place the cursor between `1` and `3`.
1. Type `2`.
1. Expect the cursor to be between `2` and `3`.
1. Observe the cursor to be after `4`.

    This happens because the value of the input gets reformatted to add the comma.

## Desired behavior

- After modifying the input (via inserting text or deleting text), the cursor position generally meets users' expectations.

- There may be some edge cases where users expectations are not crystal clear. That's okay. We just want to offer some improvement that meets the expectations under most circumstances. For example, let's say we begin with `12,345` and the cursor between `1` and `2`. Then we press forwards-delete. We end up with the text `1,345`. Where should the cursor go? Between `1` and `,` or between `,` and `3`? Either would be fine and would be an improvement over the current behavior (where the cursor ends up at the very end).

## Implementation

1. In `FormattedInput.svelte`, in the function `handleChildValueChange`, you have access to:

    - the value before re-format (as `userInput`)
    - the cursor position before re-format (as the `cursorPosition` property of the return value from `getOutcomeOfBeforeInputEvent`)
    - the value after reformat (as `parseResult.intermediateDisplay`)

1. Write a pure function that takes input of the three values above and returns the resulting cursor position after the text has changed.

1. I suggest using https://www.npmjs.com/package/fast-diff to diff the text values. Then use the old cursor position to figure out where _in the diff_ the cursor should go, and use the diff to figure out where the cursor should go in the resulting text.

Research possibility of setting inputmode when negative

cat | gh issue create -F - -t 'For numeric inputs, research the possibility of setting the inputmode attribute when allowing negative numbers' -m '[Beta] Better Editing Experience' -l "status: ready" -l "type: enhancement" -l "work: frontend"

- In In `src/component-library/number-input/numberInputUtils.ts` we have a function, `getInputMode` which derives the value of the [`inputmode` attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inputmode) to set on the `<input>` element within our `NumberInput` and `StringifiedNumberInput` components. This helps the user's device display the correct software keyboard when necessary (e.g. on a phone).

- For positive numbers, we set `inputmode` to `decimal` or `numeric` and users get a nice keyboard with big number buttons.

    But MDN says:

    > Devices may or may not show a minus key (-).

    Because of this caveat in the MDN docs, we leave `inputmode` as `text` when the input allows negative numbers. Are we being overly cautious here? Are there _actually_ devices that don't show a minus key? Which ones? Can we set `inputmode` to `decimal` or `numeric` anyway? Or will there be users that can't enter negative numbers? 

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

- Prettier handles these, so we don't need them
- I want to use some test case array blocks in my tests that break
  these rules
Define the low-level functions that we need to implement to create
generalized input formatters.
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #1155 (c0a8b38) into master (37c32d0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1155   +/-   ##
=======================================
  Coverage   93.39%   93.39%           
=======================================
  Files         113      113           
  Lines        4332     4332           
=======================================
  Hits         4046     4046           
  Misses        286      286           
Flag Coverage Δ
pytest-backend 93.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37c32d0...c0a8b38. Read the comment docs.

@seancolsen seancolsen mentioned this pull request Mar 16, 2022
7 tasks
- Handle high precision floats with StringifiedNumberFormatter.
- Handle regular numbers with NumberFormatter.
- Use AbstractNumberFormatter and makeUniversalNumberParser for
  shared logic.
Testing Library recommends this when dealing with user interactions.

I'm installing the beta version because their [docs][1] say:

> This is still a pre-release and might be subject to breaking
changes if they should be deemed necessary in the light of feedback
we receive for this version. All planned breaking changes are
already implemented though.

[1]: https://testing-library.com/docs/user-event/intro/
Safari doesn't support them
@seancolsen seancolsen marked this pull request as ready for review March 21, 2022 18:37
@seancolsen seancolsen requested review from a team and pavish March 21, 2022 18:37
@kgodey kgodey added the pr-status: review A PR awaiting review label Mar 22, 2022
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

@seancolsen Looks good to me! Nice work!

@pavish pavish merged commit c49156a into master Mar 28, 2022
@pavish pavish deleted the number_component branch March 28, 2022 18:07
@pavish
Copy link
Member

pavish commented Mar 28, 2022

@seancolsen I assume you'll create tickets mentioned in the description.

I'm going to mark #970 as closed.

@seancolsen
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants