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

Prime page - redesign #2554

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

Prime page - redesign #2554

wants to merge 10 commits into from

Conversation

kattylucy
Copy link
Collaborator

@kattylucy kattylucy commented Dec 13, 2024

@kattylucy kattylucy force-pushed the 2548_prime_page_redesign branch from b0a325f to 15fba02 Compare December 13, 2024 12:41
Copy link

github-actions bot commented Dec 13, 2024

PR deployed in Google Cloud
URL: https://pr2554-app-ff-production.k-f.dev
Commit #: c135d8a
To access the functions directly check the corresponding deploy Action

Copy link

github-actions bot commented Dec 13, 2024

PR deployed in Google Cloud
URL: https://app-pr2554.k-f.dev
Commit #: c135d8a
To access the functions directly check the corresponding deploy Action

@kattylucy kattylucy force-pushed the 2548_prime_page_redesign branch 2 times, most recently from e6b57e3 to e9f428f Compare January 9, 2025 09:40
@kattylucy kattylucy changed the base branch from main to create_pool January 10, 2025 11:05
@kattylucy kattylucy force-pushed the 2548_prime_page_redesign branch from 55d3be6 to ce03402 Compare January 10, 2025 11:05
@kattylucy kattylucy marked this pull request as ready for review January 14, 2025 16:04
Comment on lines +32 to +54
export const BackButton = ({
to,
children,
label,
align,
}: {
to: string
children?: ReactNode
label: string
align?: string
}) => {
return (
<Box display="flex" alignItems="center" width="55%" justifyContent={align || 'space-between'} mt={15} mb={24}>
<StyledRouterLinkButton to={to} small icon={<IconArrowLeft />} variant="tertiary" />
<Box display="flex" alignItems="center">
<Text variant="heading1" style={{ marginRight: 8 }}>
{label}
</Text>
{children}
</Box>
</Box>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would fit well into fabric, maybe pass the RouterLinkButton as a prop?

Copy link
Collaborator

@sophialittlejohn sophialittlejohn left a comment

Choose a reason for hiding this comment

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

Great job! I left a few comments for improvements for the code. And also have a few improvement suggestions for the chart

  1. y-axis labels: the values and their precision should depend on the how big the gaps are in between. If you look at the 30 days filter the values appear to be too close together.
  2. x-axis labels: also here we should define the label differently depending on how large the filtered dataset is. For 30 days I think we should label every day, for 90 days possibly every 3rd, anything bigger just display the month. If the data set is only 30 days total but the filter is set to 90 days, it should still show every day on the x-axis label.
  3. The chart fills in after the page has loaded, we should find a more elegant solution than letting the content jump. Reserving the space in advance is an option, also figuring out how to load the chart in advance could be an option.
  4. Could you look into fixing the awkward space between the top two horizontal lines?
Screenshot 2025-01-17 at 17 03 33

@@ -39,86 +32,45 @@ export function CardPortfolioValue({

const { colors } = useTheme()

const [range, setRange] = React.useState<(typeof rangeFilters)[number]>({ value: 'ytd', label: 'Year to date' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for removing the types? Probs best to keep them

Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it work you could improve the types of getRangeNumber()

@@ -139,6 +180,7 @@ export function useHoldings(address?: string, chainId?: number, showActions = tr
...token,
tokenPrice: token.tokenPrice.toDecimal() || Dec(0),
showActions,
connectedNetwork: wallet.connectedNetwork,
Copy link
Collaborator

Choose a reason for hiding this comment

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

connectedNetwork would just be the network the current user is connected to, I think the intention here is to show the network on which the tokens are held.

I'm not 100% sure but I would think you can grab the chain id from the account that holds the balance. You would have to extend the first part of the gql trancheBalances like this:

trancheBalances {
        nodes {
          claimableCurrency
          claimableTrancheTokens
          pendingInvestCurrency
          pendingRedeemTrancheTokens
          sumClaimedCurrency
          sumClaimedTrancheTokens
          trancheId
          account {  # add this line
            chainId  # add this line
          }          # add this line
          poolId
          tranche {
            tokenPrice
            blockchainId
          }
          pool {
            currency {
              decimals
            }
          }
        }
      }

@@ -41,8 +31,11 @@ type Row = {
network: Network
}

export function Transactions({ onlyMostRecent, narrow, txTypes, address, trancheId }: TransactionsProps) {
export function Transactions({ onlyMostRecent, narrow, txTypes, address, trancheId, title }: TransactionsProps) {
const navigate = useNavigate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed

@@ -114,6 +114,17 @@ export const validateValues = (values: CreatePoolValues) => {
})
}

if (values.issuerCategories.length > 1) {
values.issuerCategories.forEach((category, i) => {
if (category.type == '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this and the ones below should be fixed!

@@ -27,7 +27,8 @@ type BackgroundColorName = `background${
| 'Thumbnail'
| 'AccentPrimary'
| 'AccentSecondary'
| 'Inverted'}`
| 'Inverted'
| 'Light'}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you meant to add a new color to the border colors instead of background? Also any chance you could update either primary or secondary of the existing border colors instead of adding a new one? Light doesn't really fit into our naming semantics as the colors are typically defined with how they're used rather than what they are

Base automatically changed from create_pool to main January 20, 2025 10:55
@kattylucy kattylucy force-pushed the 2548_prime_page_redesign branch from 0b3c215 to c135d8a Compare January 20, 2025 12:09
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