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: tenure cost per month #269

Merged
merged 15 commits into from
Jan 17, 2025
Merged

feat: tenure cost per month #269

merged 15 commits into from
Jan 17, 2025

Conversation

gabrielegranello
Copy link
Collaborator

What does this PR do?

Adds the content to the second card. Still work in progress..

Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fairhold-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2025 1:39pm

app/components/graphs/TenureComparisonBarChart.tsx Outdated Show resolved Hide resolved
/>
<ChartTooltip content={<ChartTooltipContent hideLabel />} />
<ChartLegend content={<ChartLegendContent />} />
<Bar dataKey="monthly" strokeWidth={2} activeIndex={2} />
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to embed the LabelList component here (please see #272)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Not sure if you agree, but I slightly change the format function otherwise it was showing
4k, 2k and 630 when looking at monthly values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2025-01-10 at 17 12 29

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call - this makes more sense given the context of the second graph 👍

import { Drawer } from "../../ui/Drawer";
import { Household } from "@/app/models/Household";

interface DashboardProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a repeated interface - each card that takes in data will be the same. Could we share this or make a common component for our cards?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! I will have DahsboardProps exported when it is declared, so it can be imported on all the modules  

Comment on lines 15 to 32
// Define type for DataInput
type DataInput = {
category: string;
marketPurchase: number;
marketRent: number;
socialRent: number;
fairholdLandPurchase: number;
fairholdLandRent: number;
affordabilityMonthly: number;
[key: string]: string | number;
};

// Define the props type for StackedBarChart
interface StackedBarChartProps {
data: DataInput[];
}

// Implementation of the Chart.js Stacked Bar Chart
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Redundant / noisy comments

<Card>
<CardHeader></CardHeader>
<CardContent>
<ChartContainer config={{}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Empty config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that config is a compulsory variable to pass. We used to pass some properties in terms of colours for the plot. However, the same properties can be passed chartData. Since we actually don't need to pass any other config parameter, but we need to pass something otherwise it flags an error, I thought the solution is to pass an empty objet. Is there perhaps a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah didn't know this - if that's the case then this is great 👌

import { Household } from "@/app/models/Household";
import TenureComparisonBarChart from "./TenureComparisonBarChart";

interface TenureComparisonWrapperProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these props correct? We're only passing a household into this wrapper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point thanks for flagging it

@gabrielegranello gabrielegranello merged commit cb5cc91 into main Jan 17, 2025
5 checks passed
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