Custom Reports date paused #2080
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
packages/desktop-client/src/components/reports/ReportSidebar.js
Outdated
Show resolved
Hide resolved
| if (cond === 'live') { | ||
| onSelectRange(dateRange); | ||
| } else { | ||
| onChangeDates(startDate, endDate); | ||
| } |
There was a problem hiding this comment.
💬 suggestion: I don't quite understand why we have 2x different callbacks here. Why not unify them into a single one onDateChange/onChangeDate? Would make things easier and reduce duplication.
Surely on the consumer level you're not storing 2x different dates for the custom reports, but instead a single date (or if you are storing 2x dates - that's actually a problem that we will need to solve before we save them to the DB).
There was a problem hiding this comment.
The local usage of 2x date ranges also seems off to me, but I understand why it's done as-is (to make the implementation easier). So I'm OK with that.
There was a problem hiding this comment.
I think it was unesseccary extra code. I think it's better now.
For reference I am not saving 2 dates. Current saved fields are: (isDateStatic, dateRange, startDate, endDate). First one tells us if it's a static or live graph, if live then next one saves the dateRange for the live graph, if static the final 2 tell us the start/end date.
Concievably this could all be stored as one array if that's easier to maintain.
dateArray{isDateStatic: (true/false), dateRange: string, startDate: yyyy-mm-dd, endDate: yyyy-mm-dd}
There was a problem hiding this comment.
Those are 2x date-ranges though: 1) dateRange; 2) startDate + endDate. We should not be saving duplicate information in the DB (eventually).
You could combine them in just a single date object: startDate + endDate + isStatic. If isStatic = false then you can use endDate - startDate to calculate the dateRange value.
There was a problem hiding this comment.
How would that equation know the difference between "thisMonth" and "lastMonth"? Especially for a graph I save now then don't open again for a couple months? Seems overly complex for something that can be solved by saving it directly as a simple string.
There was a problem hiding this comment.
Good argument. I hadn't thought about the this month/last month problem. In that case it does make sense to keep all the values.
trafico-bot
bot
added
✨ Merged
* work * fixes * updates * update table layout * revert changes * notes * updae names and improve flow
Add live/static choice for date filters.