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

CentrifugeApp: Portfolio page Overview graph #1670

Merged
merged 16 commits into from
Dec 6, 2023

Conversation

sophialittlejohn
Copy link
Collaborator

@sophialittlejohn sophialittlejohn commented Oct 30, 2023

Description

This pull request portfolio overview graph on the /portfolio page.

Closes #1593

Approvals

  • Dev
  • Designer
  • Product

Screenshots

Screenshot 2023-12-04 at 7 48 58 PM

Impact

Portfolio page

@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch 2 times, most recently from 1ccbf50 to a7c1c21 Compare November 1, 2023 17:13
@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch 2 times, most recently from e7d8833 to 7aeb3c6 Compare November 20, 2023 19:18
@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch 5 times, most recently from 7d7a0a1 to 93be3b7 Compare November 30, 2023 18:14
@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch 2 times, most recently from 8c13ae4 to fc494f5 Compare December 4, 2023 18:56
@jpangelle jpangelle marked this pull request as ready for review December 5, 2023 01:46
@annamehr
Copy link
Contributor

annamehr commented Dec 5, 2023

@jpangelle Looks great let's merge it and I create a follow up ticket for the spacing.

@TimHoub
Copy link

TimHoub commented Dec 5, 2023

Some minor UI improvements that are a not in line with the design yet.

  • The 'USD' on the left side looks a bit weird and is not in the designs.
  • The active state on the filter is a little bit different. -> the non active items should have a font weight that is lighter than the active one.
  • The filter alignment should be on the same width as the right side of the graph, now it is bit too much to the right.

Apart from that, looks great!

@jpangelle
Copy link
Contributor

Some minor UI improvements that are a not in line with the design yet.

  • The 'USD' on the left side looks a bit weird and is not in the designs.

  • The active state on the filter is a little bit different. -> the non active items should have a font weight that is lighter than the active one.

  • The filter alignment should be on the same width as the right side of the graph, now it is bit too much to the right.

Apart from that, looks great!

Regarding the USD bit, this was a suggestion from Dennis in order to signify what the y-axis represents. Is there a position we can place it that would look cleaner?

@annamehr
Copy link
Contributor

annamehr commented Dec 5, 2023

@jpangelle that was Tim's suggestion for a cleaner look
image

@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch from aa6ae35 to a0a7719 Compare December 5, 2023 15:16
@jpangelle
Copy link
Contributor

@TimHoub @annamehr Your comments have been addressed.

@@ -55,11 +55,13 @@ const sorter = <T extends Record<string, any>>(data: Array<T>, order: OrderBy, s
if (order === 'asc') {
return data.sort((a, b) => {
if (sortKey === 'nftIdSortKey') return new BN(a[sortKey]).gt(new BN(b[sortKey])) ? 1 : -1
if (sortKey === 'position') return a[sortKey].gt(b[sortKey]) ? 1 : -1
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels kinda weird to have in here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the table sorting requires the sortable value to be a number, maybe you could adapt this so that the sorter checks first if the value is a Dec/BN and sort accordingly? Then you would probs be able to remove the line above it as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

) {
const today = new Date()

return Array(rangeValue + 1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

instead of calculating the range after fetching the data, would it be possible to use graphQL filters to only fetch only the range you actually need? I think you could edit the getDailyTrancheStates and in the filters to do something like

query($trancheId: String!) {
        trancheSnapshots(
          orderBy: BLOCK_NUMBER_ASC,
          filter: {
            trancheId: { includes: $trancheId },
            timestamp: { greaterThanOrEqualTo: <date to fetch from> }
          }) {
          nodes {
            timestamp
            trancheId
            ...
          }
        }
      }

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I think so but if the user toggles the selection to another range, it require another call to the subquery to get the data rather than having all of the data already. Might feel sluggish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dec-05-2023.21-13-10.mp4

Yeah, unfortunately it is a bit janky when implementing it this way.

@jpangelle jpangelle force-pushed the cent-app/pool-overview-card branch from 3443e09 to f0349e6 Compare December 6, 2023 01:32
Copy link

github-actions bot commented Dec 6, 2023

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

@jpangelle jpangelle merged commit 2f35555 into main Dec 6, 2023
7 checks passed
@jpangelle jpangelle deleted the cent-app/pool-overview-card branch December 6, 2023 15:24
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.

Portfolio page: Overview graph
5 participants