chore: migrate FormattedNumber component from jsx to tsx #17361
Conversation
|
@etr2460 ptal, thx! |
Codecov Report
@@ Coverage Diff @@
## master #17361 +/- ##
==========================================
- Coverage 77.14% 76.91% -0.23%
==========================================
Files 1036 1039 +3
Lines 55759 56185 +426
Branches 7628 7794 +166
==========================================
+ Hits 43013 43216 +203
- Misses 12490 12713 +223
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/visualizations/TimeTable/FormattedNumber.tsx
Outdated
Show resolved
Hide resolved
| if (format) { | ||
| return <span title={num}>{formatNumber(format, num)}</span>; | ||
| return ( | ||
| <span title={num as string}>{formatNumber(format, num as number)}</span> |
There was a problem hiding this comment.
it looks like formatNumber might actually support strings already (according to the old code). could you check the function in superset-ui and see if that type needs to be changed?
also, for title, let's do title={`${num}`} instead asserting and overriding the type
There was a problem hiding this comment.
it looks like
formatNumbermight actually support strings already (according to the old code). could you check the function in superset-ui and see if that type needs to be changed?
Sure thing.
also, for
title, let's dotitle={`${num}`}instead asserting and overriding the type
Will do.
There was a problem hiding this comment.
it looks like
formatNumbermight actually support strings already (according to the old code). could you check the function in superset-ui and see if that type needs to be changed?
As per the superset-ui documentation for number-format, it looks like formatNumber fallbacks to d3.format to handle the error for invalid format like string.
However, changing:
<span title={num as string}>{formatNumber(format, num as number)}</span>
to
<span title={num as string}>{formatNumber(format, num)}</span>;
in the tsx file is throwing an error:
Argument of type 'string | number' is not assignable to parameter of type 'number | null | undefined'.
Type 'string' is not assignable to type 'number | null | undefined'.
There was a problem hiding this comment.
thanks for looking into this. Since formatNumber(format, num) has been working today already, and i'm a bit hesitant to parseInt the string in case we lose accuracy for very large numbers, I think it's ok to override typescript here. Once change I might make though is insead of doing num as number (which would imply that it's always a number, even when it isn't) I would add a // @ts-expect-error formatNumber can actually accept strings, even though it's not typed as such comment to bypass the TS error
There was a problem hiding this comment.
@Damans227 @etr2460 Just to be sure here. The superset-ui documentation for number-format referenced above states the following:
It is powered by a registry to support registration of custom formatting, with fallback to d3.format and handle error for invalid format string.
It's talking about the formatting string, which is the first parameter of the function, and not about invalid formats for the num (second parameter). Are you sure the value can be interpreted as strings?
There was a problem hiding this comment.
@michael-s-molina @Damans227 so this uses d3-format under the hood, which is untyped, but as far as i can tell does support strings (see where it converts it here):
https://github.com/d3/d3-format/blob/main/src/locale.js#L74
But more importantly than that, before we allowed all strings through here, so i don't think adding a parseInt is correct since that could change the behavior. coercing the type with an as number also doesn't feel right since we don't know it's a number. That's why i recommended the options above
|
/testenv up |
|
@pkdotson Ephemeral environment spinning up at http://34.221.117.162:8080. Credentials are |
|
@pkdotson did you want to test anything here before we merge them? |
|
LGTM! |
|
Ephemeral environment shutdown and build artifacts deleted. |
* migrate FormattedNumber component from jsx to tsx * Unset default prop on format * undo asserting and overriding the type for num * Add a ts comment to ignore error
mistercrunch
added
🏷️ bot
SUMMARY
PR for migrating
FormattedNumberPropsfunctional react component from JSX to TSXBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION