Skip to content
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

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

FrancoAguzzi
Copy link
Collaborator

@FrancoAguzzi FrancoAguzzi commented Aug 21, 2024

Copy link

vercel bot commented Aug 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples.nameguard.io ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 4, 2024 9:44pm
nameguard.io 🛑 Canceled (Inspect) Nov 4, 2024 9:44pm
namehashlabs.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 4, 2024 9:44pm
storybook.namekit.io ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 4, 2024 9:44pm

Copy link

changeset-bot bot commented Aug 21, 2024

🦋 Changeset detected

Latest commit: a401367

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@namehash/namekit-react Minor
@namehash/ens-utils Minor
@namehash/nameguard-react Patch
@namehash/nameguard-js Patch
@namehash/nameguard Patch

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
@vercel vercel bot temporarily deployed to Preview – examples.nameguard.io August 21, 2024 21:10 Inactive
@vercel vercel bot temporarily deployed to Preview – namehashlabs.org August 21, 2024 21:10 Inactive
@vercel vercel bot temporarily deployed to Preview – nameguard.io August 21, 2024 21:10 Inactive
@vercel vercel bot temporarily deployed to Preview – nameguard.io August 21, 2024 21:18 Inactive
@vercel vercel bot temporarily deployed to Preview – namehashlabs.org August 21, 2024 21:18 Inactive
@vercel vercel bot temporarily deployed to Preview – examples.nameguard.io August 21, 2024 21:18 Inactive
… | Rewrote enums definitions to optimized model | Optimized Currency-Price spacings
Copy link
Member

@lightwalker-eth lightwalker-eth left a 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} />;
Copy link
Member

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 👍

Copy link
Collaborator Author

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:

Captura de Tela 2024-09-11 às 10 22 22

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:

Captura de Tela 2024-09-11 às 10 45 10

I will further test ideas here so we can delete the UsdTextSymbol component from our codebase 🙂👍🏼

Copy link
Collaborator Author

@FrancoAguzzi FrancoAguzzi Sep 11, 2024

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:

Captura de Tela 2024-09-11 às 15 08 02

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>
  );
};

Copy link
Collaborator Author

@FrancoAguzzi FrancoAguzzi Sep 11, 2024

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.

packages/namekit-react/src/components/CurrencySymbol.tsx Outdated Show resolved Hide resolved
packages/namekit-react/src/components/CurrencySymbol.tsx Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants