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

Goals: Use shared 'months' functions for time #1082

Merged
merged 6 commits into from
Jun 25, 2023

Conversation

shall0pass
Copy link
Contributor

Based on Tom's comment (#1058 (comment)) I've eliminated all direct use of date-fns and only use the functions in months.ts.

@netlify
Copy link

netlify bot commented May 31, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 70cfcf4
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/64989fcf343f1e0008abea8d
😎 Deploy Preview https://deploy-preview-1082--actualbudget.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@MatissJanis
Copy link
Member

👋 Hey! Is this PR out for review? If so - would you mind converting it from draft? And also CI is failing.

If this is still work in progress: would you mind prefixing the PR title with "[WIP]"?

Thanks!

@shall0pass
Copy link
Contributor Author

👋 Hey! Is this PR out for review? If so - would you mind converting it from draft? And also CI is failing.

If this is still work in progress: would you mind prefixing the PR title with "[WIP]"?

Thanks!

I won't have time for a couple of weeks to check for errors (vacation). I'll mark as WIP for now.

@shall0pass shall0pass changed the title Goals standardize time [WIP]Goals standardize time Jun 2, 2023
@MatissJanis
Copy link
Member

Amazing, thanks @shall0pass ! Enjoy your well deserved time off! :)

target_month = addMonths(target_month, repeat);
num_months = differenceInCalendarMonths(
target_month = monthUtils.addMonths(target_month, repeat);
num_months = monthUtils.differenceInCalendarMonths(
template_lines[l],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a situation where this code is run, but we should be passig the month parameter instead of the whole template line.

@shall0pass shall0pass changed the title [WIP]Goals standardize time Goals: Use shared 'months' functions for time Jun 15, 2023
@shall0pass shall0pass marked this pull request as ready for review June 15, 2023 11:45
Copy link
Member

@MatissJanis MatissJanis left a comment

Choose a reason for hiding this comment

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

IMO looks good! But I'd appreciate a review from someone else too since I'm not really using goal templates myself.

@j-f1 j-f1 merged commit 78e7468 into actualbudget:master Jun 25, 2023
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jun 25, 2023
TomAFrench added a commit to TomAFrench/actual that referenced this pull request Jun 26, 2023
* master:
  ⬆️  upgrade @reach/listbox and remove monkeypatch (actualbudget#1190)
  🔥  remove beta code and some unused scripts (actualbudget#1189)
  Goals: Use shared 'months' functions for time (actualbudget#1082)
  Fix completed feature request handling (actualbudget#1183)
  Fix “delete file” modal layout (actualbudget#1170)
  chore: enforce proper types in `sync/index` (actualbudget#1077)
  Fix transaction list page being blank on mobile (actualbudget#1171)
  Automatic category selection for new category transactions (actualbudget#1176)
TomAFrench added a commit to TomAFrench/actual that referenced this pull request Jun 26, 2023
* master:
  ⬆️  upgrade @reach/listbox and remove monkeypatch (actualbudget#1190)
  🔥  remove beta code and some unused scripts (actualbudget#1189)
  Goals: Use shared 'months' functions for time (actualbudget#1082)
@shall0pass shall0pass deleted the goalsStandardizeTime branch June 27, 2023 14:14
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
Based on Tom's comment
(actualbudget#1058 (comment))
I've eliminated all direct use of date-fns and only use the functions in
months.ts.

---------

Co-authored-by: Matiss Janis Aboltins <[email protected]>
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.

3 participants