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

Add daily and weekly to custom reports interval list #2483

Merged
merged 46 commits into from
Apr 16, 2024

Conversation

carkom
Copy link
Contributor

@carkom carkom commented Mar 21, 2024

Just a couple tweaks to get weekly working. Most the backend groundwork has already been laid.

Needs to "yearly" PR merged first. Done.

Weekly became more complicated as I implented user prefs to match the begining of the week for the reports

youngcw
youngcw previously approved these changes Apr 9, 2024
Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

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

@carkom can we reduce some of places we need to pass the firstDayOfWeekIdx around?

It looks like outside of loot core we could switch to importing it in reportRanges.ts, grouped-spreadsheet.ts, custom-spreadsheets.ts and reduce the need to keep passing it around in the higher level components.

@carkom
Copy link
Contributor Author

carkom commented Apr 12, 2024

@carkom can we reduce some of places we need to pass the firstDayOfWeekIdx around?

It looks like outside of loot core we could switch to importing it in reportRanges.ts, grouped-spreadsheet.ts, custom-spreadsheets.ts and reduce the need to keep passing it around in the higher level components.

Can't call it inside the spreadsheet since they are called with "useMemo" it comes back with a hooks error.

As for reportRanges I get an "invalid Hook Call" if I try to use it in the function directly.

@twk3
Copy link
Contributor

twk3 commented Apr 13, 2024

Can't call it inside the spreadsheet since they are called with "useMemo" it comes back with a hooks error.

As for reportRanges I get an "invalid Hook Call" if I try to use it in the function directly.

Ah, right. These aren't react components at this level, so we can't reference hooks in them. Apologies for the noise.

@carkom carkom requested a review from twk3 April 14, 2024 07:06
@carkom carkom requested a review from twk3 April 15, 2024 17:05
Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

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

Has been working well in my testing. 👍

@carkom carkom merged commit d9f5546 into actualbudget:master Apr 16, 2024
19 checks passed
@carkom carkom deleted the addDailyReports branch April 16, 2024 15:19
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review labels Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants