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

feat: Portfolio holdings details #278

Merged
merged 13 commits into from
Mar 5, 2024
Merged

feat: Portfolio holdings details #278

merged 13 commits into from
Mar 5, 2024

Conversation

Xavier-Charles
Copy link
Contributor

@Xavier-Charles Xavier-Charles commented Mar 4, 2024

This PR uses mock Data for now.
Screenshot 2024-03-04 at 18 25 27

@Xavier-Charles Xavier-Charles marked this pull request as ready for review March 4, 2024 17:28
Copy link
Contributor

@v-almonacid v-almonacid left a comment

Choose a reason for hiding this comment

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

LGTM

const PortfolioChart: FC = () => {
const assets = useMemo<TPortfolioDataForAddress["assets"]>(
() =>
portfolioData !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be defined as a 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.

Yeah but that's for later

subtext="Connect your wallet to get started"
icon={<CopySVG className="w-[22px] mr-1" />}
onClick={() => onClick("/portfolio/connect-wallet")}
isAuthenticated={isAuthenticated}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: we've been told today that this option was not going to be included. But it's fine to keep it for now

import { TPortfolioDataForAddress } from "src/components/portfolio/types";

export const portfolioData: TPortfolioDataForAddress = {
assets: [
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally we need to define a portoflio component API that is decoupled from the data API provider, but this is work for a separate PR.

},
];
return (
<IonPage className="justify-start portfolio-widget">
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we use some of the mobile layout templates here?

@Xavier-Charles Xavier-Charles marked this pull request as draft March 5, 2024 07:45
@Xavier-Charles Xavier-Charles marked this pull request as ready for review March 5, 2024 07:45
@Xavier-Charles Xavier-Charles merged commit d2e239c into dev Mar 5, 2024
1 check passed
@Xavier-Charles Xavier-Charles deleted the feat/portflio-details branch March 5, 2024 07:53
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