-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
Custom Reports: Include current #2684
base: master
Are you sure you want to change the base?
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
The 'current interval' controlled by the option is 'month' rather than what is defined by the interval option? if so I'd suggest renaming it to Include current month for clarity. e.g. interval is weekly, with a live range of last 3 months, this option toggles the 4 weeks starting this month. Noticed option is disabled when using a static range(makes obvious sense). Were you thinking to disable it for the interval + live ranges that it doesn't make sense for, or not worry about that? |
It's not always monthly. It's based on the range interval. Example: choosing "Last week" and Interval = daily. The toggle would give you all the days from this week as well. If we changed it to match the interval instead and you chose "last month" and interval = weekly. You'd see all 4 weeks of March. Select the new toggle and you'd get the 4th week of April - the first three weeks would be missing. Not sure that makes sense to ever have a view like that. Also, it makes the code needlessly complicated. I see your point with labeling, because it's not actually "current interval". I've changed the wording to follow the "range" selection. So it says "Include current month" when a monthly range is chosen and "Include current week" when a weekly range is chosen. |
Ah! I see where I was getting confused, thanks. :) (there was an extra layer of misunderstanding on my part too) |
@Teprifer, can you check this meets your expectations? Make sure to only use a copied budget file or a test file as the migration in this PR will cause the budget file to not work with edge. |
Awww <3 Yeah I always test in a browser data cleared chrome window with a freshly exported budget.
Besides those minor points it looks pretty good. |
@Teprifer So there's a regression from a previous PR that broke filters. This PR is not focused on fixing those at all. You can check #2785 for the fix for filters corrections. Can you edit your review (above) and take out anything that relates to filters or show activity clicks? It would be much easier if we could keep the issues separate. Cheers! |
@carkom Sorry didn't realise the filter correction stuff included the dates bit (only aware of amounts) so thought maybe related as this has a date affecting option. And the first part was linked to the new option in this PR, anyway, removed. |
I think I've fixed both issues. Have a look. Making "Year to date" and "All time" function within the "include current" logic is messy because the interval type can change. Example: when interval = Monthly currentInterval would be current month, when interval = weekly currentInterval would be current week. Making this change dynamically adds complexity that I don't see as neccessary. "Year to date" is meant to be Jan 1st through today. If someone wants Jan of this year through this month minus one then they can utilize the static filters. Same with "All time" range. It's one extra click to use these static fields. |
Ah yep, I was thinking it'd be static as month for YTD and year for all time(regardless of interval setting), but as I mentioned I don't think there's an incorrect approach here, so the simplest makes sense. Besides which I imagine it's fairly niche anyhoo. Had a look, seems fine, my brain is a bit fried, but if you're happy with it, onwards! :) |
Would it make sense to turn this option on by default? I feel like including the current would be wanted more often than not. |
I just realized, that the time range options probably need updated to consider this setting. |
That's as intended right? "Last 6 months" (eg previous 6 months) would be the 6 full (completed) months before the current month. If you tick "include current" then you'd get those same 6 months plus this month (6 + 1 = 7). The other option would be to make the "Last 6 months" (and other options like it) dynamic. So with includeCurrent selected it would give you Dec '23 to May '24 with it unselected you'd get Nov '23 to Apr '24. It could make sense either way. I find it a bit confusing that the start date changes based on if "include current" is selected. In my mind "last 6 months" is (current month - 7) through (current month - 1). The status of "includeCurrent" wouldn't change that. |
Your second option is what I expected to happen. For me the "6" part takes precedent and "last x months" could easily mean either before current or with current, so I then expect 6 months in the report. Maybe we can see what others think. |
With the option off by default the logic is correct. I think it could also be approached as a discoverabilty thing, if the include current option was right under the range then it should be obvious what's going on. Or as you say, update the range text to reference there current month being included(or not), either by changing the number, or appending "(plus current X)" or some variation of. |
Just to add my opinion. It's currently working how I would expect. I think it's behaviour is clear when you use "last month" as the example. If I select "Last month" from the live time range (and I'm graphing by time). And include current month is on. I see last month and this month. Not just this month (I think that would be worse). If I toggle current month off, I just see last month. This seems correct, and based on that, how it currently works for 'last 6 months' also makes sense to me. I get 7 months listed, just the current probably being incomplete. (Which I can remove thanks to this toggle). |
Good explanation of how my mind was working. Thanks! @youngcw, are you okay to merge as is? |
Testing an option to add/remove current interval to "live" ranges.
WARNING: Do not use live budget files with this PR as it contains a migration. Please use a copy or a test budget ONLY.
I still need to add a migration so that it can be saved as part of the report. Holding off on that right now as I don't want it to mess up anyone's budget file.