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.

Sign up for GitHub

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

Fees popover in transaction modal #317

Merged
merged 21 commits into from
Apr 30, 2024
Merged

Fees popover in transaction modal #317

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9284bfc
Fees popover in transaction modal
ioay Mar 17, 2024
3d9e425
Chakra: using tooltip instead popover component
ioay Mar 22, 2024
eb29f17
Icon tooltip styles fix
ioay Mar 22, 2024
79bd638
Naming convention & style update
ioay Apr 3, 2024
568ad88
Merge branch 'main' into show-staking-fees-in-the-dApp
ioay Apr 4, 2024
37e98bc
Styles & naming convention
ioay Apr 4, 2024
538d473
Merge branch 'main' into show-staking-fees-in-the-dApp
ioay Apr 11, 2024
d47b534
Merge branch 'main' into show-staking-fees-in-the-dApp
ioay Apr 25, 2024
745a997
Added estimated deposit fee from sdk
ioay Apr 25, 2024
c16b8fc
FeesDetails component
ioay Apr 25, 2024
a97a29a
Merge branch 'main' into show-staking-fees-in-the-dApp
ioay Apr 25, 2024
cccb0f6
Merge branch 'main' into show-staking-fees-in-the-dApp
ioay Apr 25, 2024
2f1dcad
useTransactionFee adjustment
ioay Apr 25, 2024
07ab446
Added fees calculation for UnstakeDetails
ioay Apr 25, 2024
fd65c55
Typo fix
ioay Apr 28, 2024
a5bf66b
Revert changes for TransactionDetails component
ioay Apr 28, 2024
5a5764d
Handling undefined type for selectEstimatedDepositFee
ioay Apr 28, 2024
4c642eb
Removed estimatedDepositFee from redux
ioay Apr 29, 2024
3ed9a2e
Make the `FeesTooltip` more abstract
r-czajkowski Apr 30, 2024
5d814a5
Merge branch 'main' into show-staking-fees-in-the-dApp
r-czajkowski Apr 30, 2024
0de3011
Add `TODO`
r-czajkowski Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion dapp/src/components/TransactionModal/ActiveStakingStep/StakeFormModal/StakeDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from "react"
import { List } from "@chakra-ui/react"
import TransactionDetailsAmountItem from "#/components/shared/TransactionDetails/AmountItem"
import { useTokenAmountFormValue } from "#/components/shared/TokenAmountForm/TokenAmountFormBase"
import { FeesTooltip } from "#/components/TransactionModal/FeesTooltip"
import { useTransactionDetails } from "#/hooks"
import { CurrencyType } from "#/types"

Expand Down Expand Up @@ -34,7 +35,9 @@ function StakeDetails({
}}
/>
<TransactionDetailsAmountItem
label="Protocol fee (0.01%)"
label="Fees"
sublabel="How are fees calculated?"
popover={<FeesTooltip />}
from={{
currency,
amount: details?.protocolFee,
Expand Down
5 changes: 4 additions & 1 deletion dapp/src/components/TransactionModal/ActiveUnstakingStep/UnstakeFormModal/UnstakeDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from "react"
import { List } from "@chakra-ui/react"
import TransactionDetailsAmountItem from "#/components/shared/TransactionDetails/AmountItem"
import { useTokenAmountFormValue } from "#/components/shared/TokenAmountForm/TokenAmountFormBase"
import { FeesTooltip } from "#/components/TransactionModal/FeesTooltip"
import { useTransactionDetails } from "#/hooks"
import { CurrencyType } from "#/types"

Expand All @@ -22,7 +23,9 @@ function UnstakeDetails({ currency }: { currency: CurrencyType }) {
}}
/>
<TransactionDetailsAmountItem
label="Protocol fee (0.01%)"
label="Fees"
sublabel="How are fees calculated?"
popover={<FeesTooltip />}
from={{
currency,
amount: details?.protocolFee,
Expand Down
43 changes: 43 additions & 0 deletions dapp/src/components/TransactionModal/FeesTooltip/FeesTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import React from "react"
import { Info } from "#/assets/icons"
import { Icon, Tooltip, List } from "@chakra-ui/react"
import { FeesItemType, FeesPopoverItem } from "./FeesTooltipItem"

const fees: Array<FeesItemType> = [
{
label: "Acre protocol fees",
amount: "100000",
currency: "bitcoin",
},
{
label: "tBTC bridge fees",
amount: "240000",
currency: "bitcoin",
},
{
label: "Bitcoin network fees",
amount: "200000",
currency: "bitcoin",
},
]

export function FeesTooltip() {
return (
<Tooltip
placement="right"
label={
<List spacing={0.5} minW={60}>
{fees.map((fee) => (
<FeesPopoverItem
label={fee.label}
amount={fee.amount}
currency={fee.currency}
/>
))}
kkosiorowska marked this conversation as resolved.
Show resolved Hide resolved
</List>
}
>
<Icon as={Info} ml={2} boxSize={4} cursor="pointer" color="grey.400" />
</Tooltip>
)
}
27 changes: 27 additions & 0 deletions dapp/src/components/TransactionModal/FeesTooltip/FeesTooltipItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import React from "react"
import { HStack } from "@chakra-ui/react"
import {
CurrencyBalance,
CurrencyBalanceProps,
} from "#/components/shared/CurrencyBalance"
import { TextSm } from "#/components/shared/Typography"

export type FeesItemType = CurrencyBalanceProps & {
label: string
}

export function FeesPopoverItem({ label, amount, ...props }: FeesItemType) {
kkosiorowska marked this conversation as resolved.
Show resolved Hide resolved
return (
<HStack w={60} justifyContent="space-between">
kkosiorowska marked this conversation as resolved.
Show resolved Hide resolved
<TextSm color="white">{label}</TextSm>
<CurrencyBalance
size="sm"
amount={amount}
color="gold.300"
balanceFontWeight="semibold"
symbolFontWeight="semibold"
{...props}
/>
</HStack>
)
}
1 change: 1 addition & 0 deletions dapp/src/components/TransactionModal/FeesTooltip/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./FeesTooltip"
11 changes: 9 additions & 2 deletions dapp/src/components/shared/TransactionDetails/AmountItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,22 @@ import { CurrencyBalanceWithConversion } from "../CurrencyBalanceWithConversion"
type TransactionDetailsAmountItemProps = ComponentProps<
typeof CurrencyBalanceWithConversion
> &
Pick<TransactionDetailsItemProps, "label">
Pick<TransactionDetailsItemProps, "label" | "sublabel" | "popover">

function TransactionDetailsAmountItem({
label,
sublabel,
popover,
from,
to,
}: TransactionDetailsAmountItemProps) {
return (
<TransactionDetailsItem label={label} alignItems="start">
<TransactionDetailsItem
label={label}
sublabel={sublabel}
popover={popover}
alignItems="start"
>
<Flex flexDirection="column" alignItems="end">
<CurrencyBalanceWithConversion
from={{
Expand Down
18 changes: 13 additions & 5 deletions dapp/src/components/shared/TransactionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import React from "react"
import { ListItem, ListItemProps } from "@chakra-ui/react"
import { TextMd } from "../Typography"
import { ListItem, ListItemProps, VStack } from "@chakra-ui/react"
import { TextMd, TextSm } from "../Typography"

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

function TransactionDetailsItem({
label,
sublabel,
popover,
value,
children,
...listItemProps
Expand All @@ -21,9 +25,13 @@ function TransactionDetailsItem({
alignItems="center"
{...listItemProps}
>
<TextMd fontWeight="semibold" color="grey.700">
{label}
</TextMd>
<VStack alignItems="start" gap={0}>
r-czajkowski marked this conversation as resolved.
Show resolved Hide resolved
<TextMd fontWeight="semibold" color="grey.700">
{label}
{popover}
</TextMd>
{sublabel && <TextSm color="grey.400">{sublabel}</TextSm>}
</VStack>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ioay ioay Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2024-04-04 at 11 25 58

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ioay ioay Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{value ? <TextMd color="grey.700">{value}</TextMd> : children}
</ListItem>
)
Expand Down
Loading