-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: migrate DisplayedPrice and create DisplayedPriceWithSymbol #361
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: a401367 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
…e dedicated file for DisplayedPriceWithAltPrice
… | Rewrote enums definitions to optimized model | Optimized Currency-Price spacings
…://github.com/namehash/namekit into francoaguzzi/sc-25297/migrate-displayed-price
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.
@FrancoAguzzi Thanks for updates. Reviewed and shared feedback.
/** | ||
* USD is always displayed as a text symbol, never being represented as a SVG icon. | ||
*/ | ||
return <UsdTextSymbol {...props} />; |
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 current solution here is good. I was also curious: What if we wrote this so that we returned a CurrencySymbol
(therefore it would be a recursive CurrencySymbol
) except here we would override the symbology
to be for the text symbol instead of the icon?
Maybe this could be achieved by returning a new <CurrencySymbol ...>
or perhaps we would just call getCurrencySymbology
but with a new value for symbology
?
If we did this, maybe we could fully remove the need for any UsdTextSymbol
UI component?
Appreciate your advice 👍
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 solution sounds wise
By overlooking the code related to this solution I have understood that recursively rendering CurrencySymbol
would result in duplicated span
elements. By doing a code test, this was exactly the result:
This happens because span
is rendered in CurrencySymbol
.
I also three or four different approaches to achieve the goal but the results were not the expected, so far.
Below drawn chart resumes the current state:
I will further test ideas here so we can delete the UsdTextSymbol
component from our codebase 🙂👍🏼
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 could achieve this by understanding this goal:
And arriving at this solution:
/**
*
* @param currency: Currency. The currency to get the Icon for (e.g. Currency.Eth)
* @param iconSize: CurrencyIconSize. The Icon size (width and height), in pixels.
* @returns
*/
const getCurrencyIcon = ({
currency,
iconSize = CurrencyIconSize.Small,
...props
}: {
currency: Currency;
iconSize: CurrencyIconSize;
}): JSX.Element => {
let symbology = <></>;
switch (currency) {
case Currency.Usd:
/**
* USD is always displayed as a text symbol, never being represented as a SVG icon.
*/
return (
<CurrencySymbol
{...props}
currency={currency}
symbology={CurrencySymbology.TextSymbol}
/>
);
case Currency.Usdc:
symbology = <UsdcIcon {...props} width={iconSize} height={iconSize} />;
break;
case Currency.Dai:
symbology = <DaiIcon {...props} width={iconSize} height={iconSize} />;
break;
case Currency.Weth:
symbology = <WethIcon {...props} width={iconSize} height={iconSize} />;
break;
case Currency.Eth:
symbology = <EthIcon {...props} width={iconSize} height={iconSize} />;
break;
case Currency.Gas:
symbology = <GasIcon {...props} width={iconSize} height={iconSize} />;
break;
}
return (
<span
aria-label={`${PriceCurrencyFormat[currency].ExtendedCurrencyNameSingular} symbol`}
>
{symbology}
</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.
More than that, I had to update getCurrencySymbology
to:
/**
* Returns a JSX.Element containing the currency's symbol, acronym or icon.
* @param currency: Currency. The currency to get the symbology for (e.g. Currency.Eth)
* @param iconSize: CurrencyIconSize. The size to use for Icon Symbology.
* For other symbologies it is ignored. We suggest you use props to define
* other symbologies sizes, as these are not SVGs but texts, instead. (e.g. className="myCustomClassForTextSize")
* @param symbology: CurrencySymbology. The symbology to use (e.g. CurrencySymbology.TextSymbol)
* @returns: JSX.Element. The currency's symbol, acronym or icon inside a JSX.Element where all extra props will be applied.
*/
const getCurrencySymbology = ({
currency,
symbology = CurrencySymbology.TextSymbol,
iconSize = CurrencyIconSize.Small,
...props
}: {
currency: Currency;
symbology: CurrencySymbology;
iconSize?: CurrencyIconSize;
}): JSX.Element => {
switch (symbology) {
case CurrencySymbology.Acronym:
return (
<p
{...props}
aria-label={`${PriceCurrencyFormat[currency].ExtendedCurrencyNameSingular} symbol`}
>
{PriceCurrencyFormat[currency].Acronym}
</p>
);
case CurrencySymbology.TextSymbol:
return (
<p
{...props}
aria-label={`${PriceCurrencyFormat[currency].ExtendedCurrencyNameSingular} symbol`}
>
{PriceCurrencyFormat[currency].Symbol}
</p>
);
case CurrencySymbology.Icon:
return getCurrencyIcon({ currency, iconSize, ...props });
}
};
These changes fix the previous problems and it match our goal 👍🏼
BUT: is it good?
IMO it is. All symbols returned by this function will now have a custom "alt text" set.
apps/storybook.namekit.io/stories/Namekit/CurrencySymbol.stories.tsx
Outdated
Show resolved
Hide resolved
apps/storybook.namekit.io/stories/Namekit/CurrencySymbol.stories.tsx
Outdated
Show resolved
Hide resolved
apps/storybook.namekit.io/stories/Namekit/CurrencySymbol.stories.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: lightwalker.eth <[email protected]>
Addresses both:
DisplayedPrice
into two single Price components