Use NumberFields in more places; enhance NumberField component #1926
Conversation
This file contains hidden or 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
charliepark
commented
Jan 31, 2024
| aria-describedby={tooltipText ? `${id}-label-tip` : undefined} | ||
| isDisabled={disabled} | ||
| maxValue={max ? Number(max) : undefined} | ||
| minValue={min !== undefined ? Number(min) : undefined} |
Contributor
Author
There was a problem hiding this comment.
the min !== undefined construct is necessary because if min is 0, min ? __ : __ evaluates to false and minValue is set as undefined
Contributor
Author
|
Looks like I have a few tests to update. |
Contributor
|
Does this overlap with #1812? |
Contributor
Author
|
Weird; I could swear I'd added a note to link it to #1812 last night. There is some overlap. There were a few pieces of #1812 that I picked up in #1854, but skipped over the NumberField migration. This picks some of that up. Justin had some adjustment to types and input manipulation that I didn't incorporate. I'll see if there are other pieces of that that make sense to pull over. |
charliepark
commented
Jan 31, 2024
Contributor
Author
|
Ah, just noticed the broken CI check. 👀 |
Contributor
Author
|
Just to close the loop from before, yes, this takes care of the same tasks from #1812, and we can close that PR once this is merged. |
just-be-dev
reviewed
Jan 31, 2024
Comment on lines
137
to
-171
| aria-describedby={tooltipText ? `${id}-label-tip` : undefined} | ||
| // note special handling for number fields, which produce a number | ||
| // for the calling code despite the actual input value necessarily | ||
| // being a string. | ||
| onChange={(e) => { | ||
| if (transform) { | ||
| onChange(transform(e.target.value)) | ||
| return | ||
| } | ||
| if (type === 'number') { | ||
| if (e.target.value.trim() === '') { | ||
| onChange(0) | ||
| } else if (!isNaN(e.target.valueAsNumber)) { | ||
| onChange(e.target.valueAsNumber) | ||
| } | ||
| // otherwise ignore the input. this means, for example, typing | ||
| // letters does nothing. If we instead said take anything | ||
| // that's NaN and fall back to 0, typing a letter would reset | ||
| // the field to 0, which is silly. Browsers are supposed to | ||
| // ignore non-numeric input for you anyway, but Firefox does | ||
| // not. | ||
| return | ||
| } | ||
|
|
||
| onChange(e.target.value) | ||
| onChange(transform ? transform(e.target.value) : e.target.value) | ||
| }} | ||
| value={type === 'number' ? numberToInputValue(value) : value} |
Contributor
There was a problem hiding this comment.
Ugh, I love to see it. Thank you @charliepark, this soothes my weary soul.
just-be-dev
approved these changes
Jan 31, 2024
Contributor
just-be-dev
left a comment
just-be-dev
left a comment
There was a problem hiding this comment.
Fantastic! Thank you so much for taking care of this.
3 tasks
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8 * [1a4f5d81](oxidecomputer/console@1a4f5d81) oxidecomputer/console#1910 * [34596e33](oxidecomputer/console@34596e33) oxidecomputer/console#1928 * [ff7cdb28](oxidecomputer/console@ff7cdb28) oxidecomputer/console#1926 * [efa39789](oxidecomputer/console@efa39789) oxidecomputer/console#1867 * [4bfadc02](oxidecomputer/console@4bfadc02) oxidecomputer/console#1927 * [695d3671](oxidecomputer/console@695d3671) oxidecomputer/console#1925 * [30070292](oxidecomputer/console@30070292) better path filter for local files in msw handler * [5cf4339c](oxidecomputer/console@5cf4339c) oxidecomputer/console#1916 * [a26f7c1e](oxidecomputer/console@a26f7c1e) oxidecomputer/console#1922 * [231b93ed](oxidecomputer/console@231b93ed) oxidecomputer/console#1923 * [c8364638](oxidecomputer/console@c8364638) better msw warning filter so we don't get warning noise in console * [764f7310](oxidecomputer/console@764f7310) oxidecomputer/console#1908 * [945619eb](oxidecomputer/console@945619eb) oxidecomputer/console#1921 * [e2d82a4c](oxidecomputer/console@e2d82a4c) oxidecomputer/console#1887 * [d6a67bd5](oxidecomputer/console@d6a67bd5) oxidecomputer/console#1918 * [1fb746f4](oxidecomputer/console@1fb746f4) oxidecomputer/console#1899 * [ca7c85de](oxidecomputer/console@ca7c85de) oxidecomputer/console#1917 * [28598e1d](oxidecomputer/console@28598e1d) oxidecomputer/console#1914 * [34eb478d](oxidecomputer/console@34eb478d) oxidecomputer/console#1912 * [4d693088](oxidecomputer/console@4d693088) bump vite-plugin-html to stop getting deprecation warning * [d5c39549](oxidecomputer/console@d5c39549) oxidecomputer/console#1909 * [7c6f53db](oxidecomputer/console@7c6f53db) oxidecomputer/console#1854
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8 * [1a4f5d81](oxidecomputer/console@1a4f5d81) oxidecomputer/console#1910 * [34596e33](oxidecomputer/console@34596e33) oxidecomputer/console#1928 * [ff7cdb28](oxidecomputer/console@ff7cdb28) oxidecomputer/console#1926 * [efa39789](oxidecomputer/console@efa39789) oxidecomputer/console#1867 * [4bfadc02](oxidecomputer/console@4bfadc02) oxidecomputer/console#1927 * [695d3671](oxidecomputer/console@695d3671) oxidecomputer/console#1925 * [30070292](oxidecomputer/console@30070292) better path filter for local files in msw handler * [5cf4339c](oxidecomputer/console@5cf4339c) oxidecomputer/console#1916 * [a26f7c1e](oxidecomputer/console@a26f7c1e) oxidecomputer/console#1922 * [231b93ed](oxidecomputer/console@231b93ed) oxidecomputer/console#1923 * [c8364638](oxidecomputer/console@c8364638) better msw warning filter so we don't get warning noise in console * [764f7310](oxidecomputer/console@764f7310) oxidecomputer/console#1908 * [945619eb](oxidecomputer/console@945619eb) oxidecomputer/console#1921 * [e2d82a4c](oxidecomputer/console@e2d82a4c) oxidecomputer/console#1887 * [d6a67bd5](oxidecomputer/console@d6a67bd5) oxidecomputer/console#1918 * [1fb746f4](oxidecomputer/console@1fb746f4) oxidecomputer/console#1899 * [ca7c85de](oxidecomputer/console@ca7c85de) oxidecomputer/console#1917 * [28598e1d](oxidecomputer/console@28598e1d) oxidecomputer/console#1914 * [34eb478d](oxidecomputer/console@34eb478d) oxidecomputer/console#1912 * [4d693088](oxidecomputer/console@4d693088) bump vite-plugin-html to stop getting deprecation warning * [d5c39549](oxidecomputer/console@d5c39549) oxidecomputer/console#1909 * [7c6f53db](oxidecomputer/console@7c6f53db) oxidecomputer/console#1854
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We currently have a few places in the UI where we have TextInputs with
type="number". This renders like this:We have a NumberField component that we're currently using in a few places, but could use in the other places where we currently have
type="number"… this PR converts those over:This PR also adds a small bit of logic to translate the
minandmaxvalue that we pass in as props to NumberField, converting those to theminValueandmaxValueprops that the NumberInput component is expecting. The benefit from that is that we now automatically disable the up/down arrow button in the NumberField component when the input's value is at the boundary number. If the user tries typing in a number beyond the boundary, it gets pulled back to the min/max automatically.