DSD-1905/formatting numbers #1721
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
I have some formatting requests. Please see my comments.
Additionally, there are requirements in the Jira ticket that have not been implemented, but I am not going to ask for that to be added just yet. The Design Team is making a decision about whether or not that formatting should be implemented.
There was a problem hiding this comment.
This looks good. At this point @bigfishdesign13 and I need to do a bit more investigation to see how teams can/should use it.
Something I would like to see is for the function to also work with string numbers. So that useFormatNumber(4276835) and useFormatNumber("4276835") both return the same value. I think it's definitely possible that numbers from various sources, databases or APIs, return number values as strings.
There was a problem hiding this comment.
As Edwin requested, update the function so it also supports numbers passed as strings
Ex. "123456"
With that, it would also be good to return a console warning when an alphanumeric or purely alphabetic string is passed.
Ex: "one" or "123xxx"
This is what the warning should say:
NYPL Reservoir useFormatNumber: An unsupported value was passed.
bigfishdesign13
added
the
Changes Requested
src/hooks/useFormatNumber.mdx
Outdated
|
|
||
| ## Usage | ||
|
|
||
| const formatNumber from "../../hooks/useFormatNumber"; |
There was a problem hiding this comment.
This code snippet should show the code that consuming apps can use to import the hook/function.
<Source
code={`
import {
formatNumber
} from "@nypl/design-system-react-components";
`} language="jsx" />
…-design-system into DSD-1905/format-number
src/hooks/useFormatNumber.ts
Outdated
| }; | ||
|
|
||
| const formatNumber = (num1: any, num2?: any): string | null => { | ||
| let hasWarnig = false; // Track if any warnings occurs |
src/hooks/useFormatNumber.ts
Outdated
| */ | ||
|
|
||
| // export default function useFormatNumber(num1: any, num2?: any) { | ||
| const useFormatNumber = (num1: any, num2?: any) => { |
There was a problem hiding this comment.
Instead of any, use string | number to give the args better types.
src/hooks/useFormatNumber.ts
Outdated
| // export default function useFormatNumber(num1: any, num2?: any) { | ||
| const useFormatNumber = (num1: any, num2?: any) => { | ||
| // const num1Pass = num1; | ||
| // const num2Pass = num2; |
There was a problem hiding this comment.
delete comments if you don't need it anymore
src/hooks/useFormatNumber.ts
Outdated
| )}`; | ||
| } | ||
|
|
||
| const enDash = "\u2013"; |
There was a problem hiding this comment.
You need this enDash for line 80, so declare it before it and use it there.
| expect(formatNumber("4382", "XX1234")).toEqual(null); | ||
| expect(formatNumber("4382XXX", "1234")).toEqual(null); | ||
| expect(formatNumber("4382XXX", "1234XXX")).toEqual(null); |
There was a problem hiding this comment.
Break these expectations out into their own test case and also verify that the warning is being logged.
bigfishdesign13
added
Ship It
Fixes JIRA ticket DSD-1905
This PR does the following:
How has this been tested?
Accessibility concerns or updates
Accessibility Checklist
aria-liveis used, a screenreader was used to verify the text is read.refs, focus management was reviewed.Open Questions
Checklist:
Front End Review: