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

Pool Overview redesign #2435

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Pool Overview redesign #2435

merged 5 commits into from
Oct 9, 2024

Conversation

kattylucy
Copy link
Collaborator

@kattylucy kattylucy commented Sep 6, 2024

Copy link

github-actions bot commented Sep 6, 2024

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

Copy link

github-actions bot commented Sep 6, 2024

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

Base automatically changed from add_tile_design to main September 10, 2024 14:13
@kattylucy kattylucy changed the title Style new header Pool Overview redesign Sep 10, 2024
@kattylucy kattylucy force-pushed the pool_overview_redesign branch 12 times, most recently from 66fc7a5 to 1181892 Compare September 17, 2024 15:51
@kattylucy kattylucy force-pushed the pool_overview_redesign branch from 909545e to 35bc8e6 Compare September 17, 2024 20:23
@sophialittlejohn sophialittlejohn mentioned this pull request Sep 17, 2024
1 task
@kattylucy kattylucy force-pushed the pool_overview_redesign branch from c29f491 to dbe5c48 Compare September 24, 2024 09:09
@kattylucy kattylucy marked this pull request as ready for review September 24, 2024 09:09
@kattylucy kattylucy force-pushed the pool_overview_redesign branch 2 times, most recently from 90bc09f to 4afb124 Compare September 26, 2024 10:41
const data: ChartData[] = React.useMemo(
() =>
truncatedPoolStates?.map((day) => {
truncatedPoolStates?.map((day: any) => {
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
truncatedPoolStates?.map((day: any) => {
truncatedPoolStates?.map((day) => {

better to make sure truncatedPoolStates is typed rather than casting day as any

<Box width="8px" height="8px" borderRadius="50%" backgroundColor={color} marginRight="4px" />
)

const navObj = {
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
const navObj = {
const navData = {

for consistency though out


const graphData = selectedTabIndex === 0 ? tokenData : apyData

const toggleRange = (e: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's try and avoid anys please!

</Shelf>
<Box display="flex" justifyContent="space-between" alignItems="center">
<Box display="flex" justifyContent="space-evenly">
{graphData.map((item: any, index: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too!

Comment on lines 14 to 11
type DailyPoolStateProps = {
timestamp: string
tranches: { [trancheId: string]: TrancheState }
apy?: Perquintill | undefined
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather build the type from the existing dailyPoolState using Pick or Omit so that we can more easily catch changes if they were to happen

Copy link
Collaborator

Choose a reason for hiding this comment

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

for TrancheState as well

const data = React.useMemo(() => {
const tokenData =
dailyPoolStates?.map((state) => {
return { price: state.tranches[trancheId].price?.toFloat() || 0, day: new Date(state.timestamp) }
dailyPoolStates?.map((state: DailyPoolStateProps) => {
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
dailyPoolStates?.map((state: DailyPoolStateProps) => {
dailyPoolStates?.map((state) => {

again this type should be interfered if dailyPoolStates is type properly

Copy link
Collaborator

Choose a reason for hiding this comment

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

now that its correctly inferred you can remove the DailyPoolStateProps from this map

import { Spinner } from '../Spinner'
import { Tooltips } from '../Tooltips'

const StyledPillButton = styled(PillButton)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems this can be removed?

Comment on lines 33 to 34
interface DailyTrancheState {
yield30DaysAnnualized: Perquintill
timestamp: string
}

function hasValuationMethod(pricing: any): pricing is { valuationMethod: string } {
return pricing && typeof pricing.valuationMethod === 'string'
type DailyTrancheStateArr = Record<string, DailyTrancheState[]>

type Tranche = {
currency: {
name: string
}
id: string
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here best to build the types from existing ones rather than redeclaring, recommend using Pick or Omit

const fmk = useFormikContext<PoolMetadataInput>()
const { values } = fmk

console.log('FORM', values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

oops

@kattylucy kattylucy requested review from sophialittlejohn and removed request for onnovisser and sophialittlejohn October 2, 2024 08:45
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.

Very nice work @kattylucy!! Just a few more comments about the types and then I think we're good to go 🔥

] as const
]

function calculateTranchePrices(pool: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would type this as Pool. Will save you having to assign types in the subsequent maps

return { juniorTokenPrice, seniorTokenPrice }
}

function getYieldFieldForFilter(tranche: any, filter: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would be tranche: Pool['tranches'][0]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I think it has to be DailyTrancheState

const data: ChartData[] = React.useMemo(
() =>
truncatedPoolStates?.map((day) => {
truncatedPoolStates?.map((day: DailyPoolState) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to have the types on truncatedPoolStates instead of type casting in the map

const data = React.useMemo(() => {
const tokenData =
dailyPoolStates?.map((state) => {
return { price: state.tranches[trancheId].price?.toFloat() || 0, day: new Date(state.timestamp) }
dailyPoolStates?.map((state: DailyPoolStateProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

now that its correctly inferred you can remove the DailyPoolStateProps from this map


{!!values?.issuerCategories?.length &&
values.issuerCategories.map(
(category: { type: string; value: string | number; customType?: string }, index: number) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

another case where types should already be inferred from the formik values, so you shouldn't have to type them manually here. If the compiler is complaining you probably need to type the values from formik first!

@kattylucy kattylucy force-pushed the pool_overview_redesign branch from 65fd070 to b636cf7 Compare October 8, 2024 08:24
@kattylucy kattylucy force-pushed the pool_overview_redesign branch from 86f6fbb to 178af27 Compare October 9, 2024 07:33
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.

🔥 🔥 🔥

@kattylucy kattylucy merged commit 180f9ec into main Oct 9, 2024
12 checks passed
@kattylucy kattylucy deleted the pool_overview_redesign branch October 9, 2024 18:01
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