feat: migrate DisplayedPrice and create DisplayedPriceWithSymbol#361
feat: migrate DisplayedPrice and create DisplayedPriceWithSymbol#361FrancoAguzzi wants to merge 47 commits intomainfrom
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
lightwalker-eth
left a comment
There was a problem hiding this comment.
@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.
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.
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.
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.
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 <126201998+lightwalker-eth@users.noreply.github.com>



Addresses both:
DisplayedPriceinto two single Price components