-
Notifications
You must be signed in to change notification settings - Fork 16
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
Shortened amount props for ValueViewComponent #1915
Conversation
🦋 Changeset detectedLatest commit: 8737509 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
0c29102
to
955e09a
Compare
Visit the preview URL for this PR (updated for commit 8737509): https://penumbra-ui-preview--pr1915-value-view-additions-molvhrc8.web.app (expires Wed, 27 Nov 2024 15:04:43 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
export function round({ value, decimals, roundingMode = 'round' }: RoundOptions): string { | ||
const roundingFn = roundingStrategies[roundingMode]; | ||
const roundedNumber = roundingFn(value, decimals); | ||
return roundedNumber.toFixed(decimals); | ||
} |
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.
Both round()
and shortify()
come from the dex explorer. However, expanded these quite a bit w/ testing as well.
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.
If this works, after publishing, will delete the local one in the dex explorer
packages/ui/src/ValueView/index.tsx
Outdated
/** | ||
* If true, the asset icon will be hidden. | ||
*/ | ||
hideIcon?: boolean; | ||
/** | ||
* If true, the asset symbol will be hidden. | ||
*/ | ||
hideSymbol?: boolean; | ||
/** | ||
* If true, the displayed amount will be shortened. | ||
*/ | ||
abbreviate?: boolean; |
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.
I'm not entirely sure if these kinds of props are in the spirit of the v2 UI component prop style
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.
Also these design changes should be validated on the storybook preview link in the main page of the PR
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.
comment: validated storybook props in preview
<div className='shrink grow' title={formattedAmount}> | ||
<ValueText density={density}>{formattedAmount}</ValueText> |
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.
Bug fix, the min width made it too wide
packages/ui/src/ValueView/index.tsx
Outdated
<span | ||
className={cn( | ||
density === 'sparse' ? technical : detailTechnical, | ||
priority === 'primary' ? 'text-text-primary' : 'text-secondary-dark', | ||
)} | ||
> | ||
{children} | ||
</span> |
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.
Needed to have this else branch as the Pill was giving the font/color in the other branch and this one wasn't getting any.
packages/ui/src/ValueView/index.tsx
Outdated
/** | ||
* If true, the asset icon will be hidden. | ||
*/ | ||
hideIcon?: boolean; | ||
/** | ||
* If true, the asset symbol will be hidden. | ||
*/ | ||
hideSymbol?: boolean; | ||
/** | ||
* If true, the displayed amount will be shortened. | ||
*/ | ||
abbreviate?: boolean; |
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.
comment: validated storybook props in preview
const formattedAmount = getFormattedAmtFromValueView(valueView, true); | ||
let formattedAmount: string; | ||
if (abbreviate) { | ||
const amount = getFormattedAmtFromValueView(valueView, false); |
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.
comment: what if getFormattedAmtFromValueView
returns something unparseable (eg. null, undefined)? should we add validation guards to shortify to prevent breakage?
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.
The function returns a string
type, so all good.
</div> | ||
<div className='min-w-[50px] shrink grow truncate' title={symbol}> | ||
<ValueText density={density}>{symbol}</ValueText> | ||
<div className='shrink grow' title={formattedAmount}> |
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.
nit; using both shrink-0
and shrink
in the same contexts.
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.
The shrink-0
prevents the icon from shrinking, The uses of shrink
on the other two say that is' ok to shrink.
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.
Going to add a few more tweaks to better support the compact version
const formattedAmount = getFormattedAmtFromValueView(valueView, true); | ||
let formattedAmount: string; | ||
if (abbreviate) { | ||
const amount = getFormattedAmtFromValueView(valueView, false); |
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.
The function returns a string
type, so all good.
</div> | ||
<div className='min-w-[50px] shrink grow truncate' title={symbol}> | ||
<ValueText density={density}>{symbol}</ValueText> | ||
<div className='shrink grow' title={formattedAmount}> |
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.
The shrink-0
prevents the icon from shrinking, The uses of shrink
on the other two say that is' ok to shrink.
Updating
ValueViewComponent
to support displaying for this view on the dex explorer:At the moment those display raw numbers, but given the metadata of the assets, we need them to be
ValueViewComponent
to handle the display exponents.