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

Assets page redesign #2513

Merged
merged 21 commits into from
Nov 6, 2024
Merged

Assets page redesign #2513

merged 21 commits into from
Nov 6, 2024

Conversation

kattylucy
Copy link
Collaborator

  • Redesign to asset overview page & asset detail page

#2507

  • Dev

Copy link

github-actions bot commented Oct 21, 2024

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

Copy link

github-actions bot commented Oct 21, 2024

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

@kattylucy kattylucy force-pushed the 2507_assets_redesign branch 6 times, most recently from ba48e5a to e182f0f Compare October 24, 2024 09:08
@kattylucy kattylucy changed the base branch from main to 2505_post_launch_fixes October 24, 2024 09:08
@kattylucy kattylucy force-pushed the 2507_assets_redesign branch 2 times, most recently from b68c433 to f7969b6 Compare October 24, 2024 09:56
@kattylucy kattylucy marked this pull request as ready for review October 24, 2024 09:57
@kattylucy kattylucy force-pushed the 2507_assets_redesign branch 3 times, most recently from b1d79bd to c232ea0 Compare October 25, 2024 08:23
@kattylucy kattylucy force-pushed the 2505_post_launch_fixes branch from 226fd0a to d17153c Compare October 25, 2024 09:53
@kattylucy kattylucy force-pushed the 2507_assets_redesign branch 3 times, most recently from a7d1cb4 to f05c566 Compare October 29, 2024 09:54
Comment on lines 23 to 24
marginLeft={[2, 2, 2, 2, 5]}
marginRight={[2, 2, 2, 2, 5]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
marginLeft={[2, 2, 2, 2, 5]}
marginRight={[2, 2, 2, 2, 5]}
mx={[2, 2, 2, 2, 5]}

{payload[0].payload.historicPV
? formatBalance(payload[0].payload.historicPV, 'USD', 2)
? formatBalance(payload[0].payload.historicPV, pool.currency.name, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be pool.currency.symbol for all of them

borderTopStyle="solid"
borderTopColor="borderPrimary"
>
<Stack as="section" padding={2} marginLeft={[0, 0, 0, 1, 3]} marginRight={[0, 0, 0, 1, 3]} gap={3}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Stack as="section" padding={2} marginLeft={[0, 0, 0, 1, 3]} marginRight={[0, 0, 0, 1, 3]} gap={3}>
<Stack as="section" padding={2} mx={[0, 0, 0, 1, 3]} gap={3}>

<Stack gap={1}>
<Shelf gap={1}>
<Box
padding="24px 16px"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding="24px 16px"
px={3}
py={2}

prefer theme values over hardcoded ones

font-family: Inter, sans-serif;
`

const LoanOption: React.FC<LoanOptionProps> = ({ loan }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

React.FC is obsolete. Normal functions will do

@@ -26,7 +26,7 @@ export function InlineFeedback({ status = 'default', children }: InlineFeedbackP
<Text variant="body3">
<Shelf alignItems="baseline" gap="4px">
<StyledIconWrapper minWidth="iconSmall" height="iconSmall" flex="0 0 auto">
<StyledIcon as={icons[status]} size="iconSmall" color={`status${capitalizeFirstLetter(status)}`} />
<StyledIcon as={icons[status]} size={24} color={`status${capitalizeFirstLetter(status)}`} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<StyledIcon as={icons[status]} size={24} color={`status${capitalizeFirstLetter(status)}`} />
<StyledIcon as={icons[status]} size="iconMedium" color={`status${capitalizeFirstLetter(status)}`} />

@onnovisser
Copy link
Collaborator

Looking really nice!

@kattylucy kattylucy requested a review from onnovisser October 29, 2024 14:04
Base automatically changed from 2505_post_launch_fixes to main October 30, 2024 08:59
onnovisser
onnovisser previously approved these changes Oct 30, 2024
@kattylucy kattylucy requested a review from onnovisser October 30, 2024 15:56
@kattylucy kattylucy dismissed onnovisser’s stale review October 30, 2024 15:57

Code changes after code was approved

@kattylucy kattylucy removed the request for review from onnovisser October 30, 2024 18:34
@kattylucy kattylucy requested a review from onnovisser November 5, 2024 13:46
Comment on lines +120 to +133
nftIdSortKey: loan.asset.nftId,
idSortKey: parseInt(loan.id, 10),
outstandingDebtSortKey: loan.status !== 'Closed' && loan?.outstandingDebt?.toDecimal().toNumber(),
originationDateSortKey:
loan.status === 'Active' &&
loan?.originationDate &&
'interestRate' in loan.pricing &&
!loan?.pricing.interestRate?.isZero() &&
!loan?.totalBorrowed?.isZero()
? loan.originationDate
: '',
maturityDate: loan.pricing.maturityDate,
portfolioPercentage,
...loan,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I noticed that the column sorting doesn't work yet. The columns that are not shown anymore in the table don't need a sortKey anymore. Instead you should replace the sortKeys with all of the sortable columns in the table. Under the hood anything that needs to be sortable needs to have its own key

'Portfolio %': loan.portfolioPercentage ? loan.portfolioPercentage : '-',
}
})
}, [rows])
Copy link
Collaborator

Choose a reason for hiding this comment

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

would add the suggested dependency pool here

@@ -342,6 +342,10 @@ export const tooltipText = {
label: 'Expense ratio',
body: 'The operating expenses of the fund as a percentage of the total NAV',
},
totalNavMinus: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
totalNavMinus: {
totalNavMinusFees: {

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.

Nice work 🔥 🔥 🔥 besides the table sorting now working right yet I think it is good enough to ship 🚢 proper table sorting can be handled in a follow up PR if it will block the release

@hieronx
Copy link
Contributor

hieronx commented Nov 6, 2024

@kattylucy Looking great! One thing I noticed: there is now a new asset with 0 value. The market price shows up as USDC which should be 0 USDC, and likewise for both P&L columns.

Screenshot 2024-11-06 at 09 02 27

I also asked Brian to come up with a loading state: https://www.figma.com/design/ng7qdNcSCXSDA6ZUdWIs6u/Pool-Overview%2F-Pool-Detail?node-id=4516-2623&t=DshORdTnFD448TSr-4 This would be nice to implement while the data in the table is loading (and a pattern we should start adopting across the app). But consider this a very nice-to-have, happy to write a separate ticket for this as well.

@hieronx
Copy link
Contributor

hieronx commented Nov 6, 2024

Also one smaller point, the download icon is off:

Screenshot 2024-11-06 at 09 06 08

Compared to design:

Screenshot 2024-11-06 at 09 06 22

@kattylucy kattylucy force-pushed the 2507_assets_redesign branch from e008388 to 1a21778 Compare November 6, 2024 10:17
@kattylucy kattylucy merged commit bf5f6a9 into main Nov 6, 2024
9 checks passed
@kattylucy kattylucy deleted the 2507_assets_redesign branch November 6, 2024 10:19
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.

4 participants