-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🧹 Round credits values on credit consumption page #21873
Conversation
<Tooltip | ||
cursor={{ fill: chartHoverFill }} | ||
formatter={(value: number) => { | ||
return [<FormattedNumber value={value} />, yLabel]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-intuitive. Rechart renders this as:
`${yLabel}: ${value}`
They need to be returned in this order ([value, name]
) to use their built-in Tooltip component. Previously, we just rendered `Value : ${value}` here. Given the nature of bar graphs, it seems safe to label them how I have here and use the actual unit/label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this has been such an eyesore for me 🙂 Only non-blocking comments from my side
Cell: ({ cell }: CellProps<FullTableProps>) => <UsageValue>{cell.value}</UsageValue>, | ||
Cell: ({ cell }: CellProps<FullTableProps>) => ( | ||
<UsageValue> | ||
<FormattedNumber value={cell.value} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider whether to use minimumFractionDigits
/maximumFractionDigits
to always show a set number of decimal points? Not sure I have a strong opinion, just curious if that might look better for consistency (e.g. 0.00
instead of 0
). IMO 2 decimal digits should be enough (.01 credit = $0.02, no need to go much more granular than that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I was also on the fence about it but the existing usage (credits remaining) didn't do that. I'll swap them all out!
@@ -142,7 +142,11 @@ const UsagePerConnectionTable: React.FC<UsagePerConnectionTableProps> = ({ credi | |||
accessor: "creditsConsumed", | |||
collapse: true, | |||
customPadding: { right: 0 }, | |||
Cell: ({ cell }: CellProps<FullTableProps>) => <UsageValue>{cell.value}</UsageValue>, | |||
Cell: ({ cell }: CellProps<FullTableProps>) => ( | |||
<UsageValue> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe get rid of the styled component here while we're in this file? 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! There's still some things with alignment that I'd like to tackle, but those are in the <Table/>
component and don't belong in this PR.
f5cd4a2
to
0d9eab0
Compare
What
closes https://github.com/airbytehq/airbyte-cloud/issues/4121
Rounds credit data to the approriate place per Intl standards.
How
Utilizes
<FormattedNumber />
from React-Intl in the table as well as in the bar graph component.Recommended reading order
any