Add expenses dashboard component #44
Conversation
| import { Container } from "./style"; | ||
| import PieChart from "../../../components/Charts/doughnut"; | ||
|
|
||
| const months = [ |
There was a problem hiding this comment.
Consider taking this out of the component. I would suggest renaming it to MONTHS and placing it under Dashboard.constants.js with a named export.
| let currentMonthExpenses = expenses | ||
| .filter( | ||
| (expense) => | ||
| moment.utc(expense.spentAt).format("MMMM YYYY") === |
There was a problem hiding this comment.
moment seems to be an overkill, we can achieve the same result with native Intl.DateTimeFormat. Something like this should work just fine:
const formatDate = (date) => {
const options = { month: "short", day: "numeric", year: "numeric" };
return new Intl.DateTimeFormat("en-US", options).format(new Date(date));
};
| <ExpenseString> | ||
| <ColumnContainer> | ||
| <ExpenseCategory> | ||
| {expense.expenseCategory.name.length > 16 |
There was a problem hiding this comment.
I would suggest extracting logic of truncation into a helper function
const truncateString = (string, maxLength = 16) =>
string.length > maxLength ? `${string.slice(0, maxLength)}...` : string;
<ExpenseCategory>{truncateString(expense.expenseCategory.name)}</ExpenseCategory>
| </ExpenseDate> | ||
| </ColumnContainer> | ||
| <ExpenseAmount> | ||
| {`${currency} |
There was a problem hiding this comment.
Consider extracting currency formatting logic outside of the JSX with a helper function.
const formatCurrency = (amount, currency) =>
`${currency} ${parseFloat(amount).toFixed(2)}`;
| <FixedDataContainer> | ||
| <> | ||
| <Heading>{`${months[monthIndex]} ${year}`}</Heading> | ||
| {currentMonthExpenses.map((expense, index) => ( |
There was a problem hiding this comment.
Consider destructuring expense properties in the map.
{currentMonthExpenses.map(({ spentAt, expenseCategory, amount }, index) => ( ...
|
|
||
| return ( | ||
| <Container> | ||
| <DataContainer> |
There was a problem hiding this comment.
I understand that <DataContainer> is intended to hold the <PieChart>, I'd suggest renaming it to something like ChartContainer for semantic clarity.
| <> | ||
| <Heading>{`${months[monthIndex]} ${year}`}</Heading> | ||
| {currentMonthExpenses.map((expense, index) => ( | ||
| <div key={index}> |
There was a problem hiding this comment.
Do not use index as a key (see https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key for more info). I would suggest to use expense.id instead.
| "December", | ||
| ]; | ||
|
|
||
| const currentYear = new Date().getFullYear(); |
There was a problem hiding this comment.
Consider creating a single Date instance and reusing it for both getFullYear() and getMonth().
const currentDate = new Date();
const currentYear = currentDate.getFullYear();
const currentMonthIndex = currentDate.getMonth();
| const currentMonthIndex = new Date().getMonth(); | ||
|
|
||
| const Expenses = ({ expenses, currency }) => { | ||
| const [year, setYear] = useState(currentYear); |
There was a problem hiding this comment.
It's not just any year, but currentYear consider renaming state vars accordingly.
| <ExpenseString> | ||
| <ColumnContainer> | ||
| <ExpenseCategory> | ||
| {expense.expenseCategory.name.length > 16 |
There was a problem hiding this comment.
The maximum length of the title is indeed 16 symbols, however, the requirement for this component states, that when the length is exceeded not only do we show the truncated title, but on hover user can also see a Popover component with the full title. https://impensa.atlassian.net/browse/KDE-10.
I would suggest to integrate this component here https://ui.shadcn.com/docs/components/popover
This PR introduces
Dashboardcomponent.