Fees popover in transaction modal

[ioay]

Refs: #295
Depends on: ##307

---

Show staking fees in the dApp

---

What was done:

• added FeesPopover component to TransactionModal
• updated TransactionDetailsAmountItem to accept additional props: sublabel & popover to pass required data
• fees item is based on mocked data

---

Preview:

---

Additional info:

Figma: https://www.figma.com/file/OUdSfHsmgV8EJpWxAzXjl5/ACRE?type=design&node-id=7398-55794&mode=design&t=3gtbEQAUkkNjhYf2-4

[netlify]

✅ Deploy Preview for acre-dapp-testnet ready!

Name	Link
🔨 Latest commit	0de3011
🔍 Latest deploy log	https://app.netlify.com/sites/acre-dapp-testnet/deploys/6630b8286a43980008743f04
😎 Deploy Preview	https://deploy-preview-317--acre-dapp-testnet.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[kkosiorowska]

I would like to make sure and keep some consistency. Do we need it? 🤔 In other places where we use tooltip, we do not change the cursor.

acre/dapp/src/pages/OverviewPage/PositionDetails.tsx

Lines 33 to 35 in 46d2d42

	<Tooltip label="Template" placement="top">
	<Icon as={Info} color="grey.700" />
	</Tooltip>

@SorinQ Please confirm how the cursor should look.

Screen.Recording.2024-04-03.at.08.33.44.mov

[SorinQ]

Yes, this is great! Thank you @kkosiorowska

[ioay]

@SorinQ Just to confirm, because I don't get it is it great as it is, or should this hand-pointer be removed on the hover info icon? I set the hand-pointer cursor in this PR, in other places we use the default arrow-cursor.

[kkosiorowska]

I wonder if this is the best solution for sure. What if for the next list element we want to apply another custom label? This could create a mess. What do you think about allowing label customization? I'm thinking of something like this:

label: React.ReactNode

Suggested change

	<VStack alignItems="start" gap={0}>
	<TextMd fontWeight="semibold" color="grey.700">
	{label}
	{popover}
	</TextMd>
	{sublabel && <TextSm color="grey.400">{sublabel}</TextSm>}
	</VStack>
	{typeof label === "string" ? (
	<TextMd fontWeight="semibold" color="grey.700">
	{label}
	</TextMd>
	) : (
	label
	)}

Then we can pass the following label:

<TransactionDetailsAmountItem
  label={
    <Flex flexDirection="column">
      <HStack>
        <TextMd fontWeight="semibold" color="grey.700">
          Fees
        </TextMd>
        <FeesPopover />
      </HStack>
      <TextSm color="grey.400">How are fees calculated?</TextSm>
    </Flex>
  }

...

/>

What do you think about this solution?

[ioay]

Your solution is good in the case when we need a more complex, unique UI. My solution can be easier to implement for other cases using sublabel and popover just passing appropriate, simple props, instead of passing:

 <Flex flexDirection="column">
      <HStack>
        <TextMd fontWeight="semibold" color="grey.700">
          Fees
        </TextMd>
        <FeesPopover />
      </HStack>
      <TextSm color="grey.400">How are fees calculated?</TextSm>
    </Flex>

every time when we need a sublabel or popover.
Let's leave it as it is, and if the need arises, we will consider extending this solution.

[kkosiorowska]

hm @r-czajkowski I'm curious what you think about it.

[kkosiorowska]

Also, If we stay with your approach I think we should also rename the popover to something more generic. Currently, we pass the component that is a tooltip which can be confusing.

acre/dapp/src/components/TransactionModal/ActiveStakingStep/StakeFormModal/StakeDetails.tsx

Line 40 in 79bd638

popover={<FeesTooltip />}

Additionally, the icon is not centered relative to the text.

[ioay]

Actually, it was broken after the last change(https://github.com/thesis/acre/pull/317/files/eb29f17f128688667324dbb90a289b6841e8c397#r1549007304), it has already been fixed - 37e98bc

[r-czajkowski]

At this moment looks like this component is only used in ActiveStakingStep. So I think we can even keep it in shared/TransactionModal/ActiveStakinStep/FeesTransacionItem.tsx.

[r-czajkowski]

TransactionDetails/index.ts

import React from "react"
import { ListItem, ListItemProps, VStack } from "@chakra-ui/react"
import { TextMd } from "../Typography"

export type TransactionDetailsItemProps = {
  label: string
  value?: string
  children?: React.ReactNode
} & ListItemProps

export function TransactionDetailsWrapper({
  children,
  ...listItemProps
}: ListItemProps & { children?: React.ReactNode }) {
  return (
    <ListItem
      display="flex"
      justifyContent="space-between"
      alignItems="center"
      {...listItemProps}
    >
      {children}
    </ListItem>
  )
}

export function TransactionDetailsLabel({ children }) {
  return (
    <TextMd fontWeight="semibold" color="grey.700">
      {children}
    </TextMd>
  )
}

function TransactionDetailsItem({
  label,
  value,
  children,
  ...listItemProps
}: TransactionDetailsItemProps) {
  return (
    <TransactionDetailsWrapper {...listItemProps}>
      <TransactionDetailsLabel>{label}</TransactionDetailsLabel>
      {value ? <TextMd color="grey.700">{value}</TextMd> : children}
    </TransactionDetailsWrapper>
  )
}

export default TransactionDetailsItem

New component

export function StakingFeesTransactionDetailsItem({ label }) {
  return (
    <TransactionDetailsWrapper>
      <VStack alignItems="start" gap={0}>
        <TransactionDetailsLabel>
          Fees
          <FeePopover />
        </TransactionDetailsLabel>
        <TextSm color="grey.400">How are fees calculated</TextSm>
      </VStack>
      <Flex flexDirection="column" alignItems="end">
        <CurrencyBalanceWithConversion
          from={{
            size: "md",
            ...from,
          }}
          to={{
            size: "sm",
            fontWeight: "medium",
            color: "grey.500",
            ...to,
          }}
        />
      </Flex>
    </TransactionDetailsWrapper>
  )
}

we can try to extract this part:

      <Flex flexDirection="column" alignItems="end">
        <CurrencyBalanceWithConversion
          from={{
            size: "md",
            ...from,
          }}
          to={{
            size: "sm",
            fontWeight: "medium",
            color: "grey.500",
            ...to,
          }}
        />
      </Flex>

since we repeat it in AmountItem

[ioay]

Thanks for the feedback @r-czajkowski ! I am also a fan of generic components, hence my suggestion to use optional props such as sublabel/tooltip in TransactionDetailsItem. Thanks to this, we can support similar solutions in the future, not only for fees and this fees tooltip:

  <TransactionDetailsItem
       label={label}
       sublabel={sublabel}
       tooltip={tooltip}
       alignItems="start"
     >

This solution allow us to freely and easily pass any sublabel or tooltip according to design without the risk of any regression which gives us a generic approach.
The sublabel can be any text and the tooltip can be any tooltip component we want to show.

Correct me if I understood wrong, but creating the StakingFeesTransactionDetailsItem component according to your proposal is not generic, but rather very restrictive only for this Fee case:

export function StakingFeesTransactionDetailsItem({ label }) {
  return (
    <TransactionDetailsWrapper>
      <VStack alignItems="start" gap={0}>
        <TransactionDetailsLabel>
          Fees
          <FeePopover />
        </TransactionDetailsLabel>
        <TextSm color="grey.400">How are fees calculated</TextSm>
      </VStack>
      <Flex flexDirection="column" alignItems="end">
        <CurrencyBalanceWithConversion
          from={{
            size: "md",
            ...from,
          }}
          to={{
            size: "sm",
            fontWeight: "medium",
            color: "grey.500",
            ...to,
          }}
        />
      </Flex>
    </TransactionDetailsWrapper>
  )
}

Maybe let's combine our suggestions and create an independent component like TransactionDetailsTooltipItem, which will accept the tooltip and sublabel as a prop and at the same time the current component TransactionDetailsItem will remain unchanged. What do you think?

[r-czajkowski]

My point is that we should avoid props like tooltip, sublabel because we may end up with meaningless props like rightLabel or leftSublabel or subValue. So I propose to split the main component into smaller components that allow as to build more specific components.

Yes StakingFeesTransactionDetailsItem is not generic it is a specific component for a given case and has only one prop label instead of tooltip, sublabel etc.

This is only my suggestion and I prefer this style for creating components but ofc we can use this approach. I'm just not convinced about the flexibility of this approach. E.g. if the designer adds another element to the designs will we add another prop?

[ioay]

Okey dokey, I restored the transaction details component to its original shape and created a similar one for fees:
c16b8fc

[kkosiorowska]

It's a list of items. So let's use the List component here. I have one more thought. The FeesPopoverItem component is very similar to TransactionDetailsAmountItem. I feel that we are duplicating the code unnecessarily.

We display the details of the transaction fees, so I think we can use the TransactionDetailsAmountItem component. It could look something like this:

        <List spacing={0.5} minW={60}>
          {fees.map((fee) => (
            <TransactionDetailsAmountItem
              key={fee.label}
              label={<TextSm color="white">{fee.label}</TextSm>}
              from={{
                currency: fee.currency,
                amount: fee.amount,
                size: "sm",
                color: "gold.300",
                fontWeight: "semibold",
              }}
            />
          ))}
        </List>

We would only need to update the TransactionDetailsAmountItem component to allow it to display the amount without conversion. What do you think about it?

[ioay]

List instead VStack added 👍🏻 I considered this solution of reusing the existing TransactionDetailsAmountItem component and making it more generic, but I think it's better to create a separate view component with pure logic instead of updating already used in other places the view component with more complex and conditional logic. Moreover, If any detail changes, there is no risk of regression in the application.

[kkosiorowska]

hm @r-czajkowski I'm curious what you think about it.

[kkosiorowska]

Also, If we stay with your approach I think we should also rename the popover to something more generic. Currently, we pass the component that is a tooltip which can be confusing.

acre/dapp/src/components/TransactionModal/ActiveStakingStep/StakeFormModal/StakeDetails.tsx

Line 40 in 79bd638

popover={<FeesTooltip />}

Additionally, the icon is not centered relative to the text.

[r-czajkowski]

Fees are not displayed in the tooltip.