-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix modals on mobile BudgetTable #3487
Conversation
WalkthroughThe changes introduced a new variable, Changes
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (7 hunks)
Additional comments not posted (3)
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (3)
228-228
: LGTM!The introduction of
modalBudgetType
to normalize the budget type used in modal references looks good. The logic for setting its value based onbudgetType
is correct.
230-230
: LGTM!Updating
categoryBudgetMenuModal
to usemodalBudgetType
instead ofbudgetType
is the correct approach. It ensures consistency with the new logic introduced bymodalBudgetType
.
402-402
: LGTM!Updating the modal references to use
modalBudgetType
instead ofbudgetType
in theonCarryover
andonTransfer
functions is the correct approach. It ensures consistency with the new logic introduced bymodalBudgetType
.Also applies to: 404-404
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
Outdated
Show resolved
Hide resolved
I think we need to handle the changing of the prefs from rollover/report to envelope/tracking. Maybe migration or setting the budget type pref to envelope/tracking accordingly. |
I think Matiss is waiting to change the pref name and migrate it to a synced pref all at once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
Yes: I would like to get rid of the rollover/report
pref value, but that will need a migration script. I plan on doing that for v24.12.0 release. In the meantime your fix is appreciated!
* marked files for translation * added release note * fixed linting warnings * 🐛 fix account filters being overridden (#3441) *Reduce package size (#3443) * reduce package size of all packages * release notes * Update beforePackHook.ts * [Maintenance] Cleanup react aria packages and dedupe (#3450) * Cleanup react aria packages and dedupe * Release notes * ♻️ (synced-prefs) moving the prefs from metadata.json to the db (#3423) *
Restart server silently when adding self signed cert and add some logs (#3431) * restart server silently on add self signed cert and add some logging * release notes * fix name * updating names to be more specific * removing setloading * feedback * ♻️ (synced-prefs) move budget type to synced prefs (#3427) * update synced account balance in db if available (#3452) * 🐛 (synced-prefs) fix bulk-reading not working in import modal (#3460) * fix: csv import deduplication (#3456) * ✨ release simplefin as a first-party feature (#3459) Closes #2272 * Do not allow renaming to an empty category or group name (#3463) * Do not allow renaming to an emoty category or group name * Release notes * [Mobile] Fix #3214 - Pull down to refresh triggering clicks on budget cells (#3374) * Fix #3214 * Fix rollover indicator * VRT * Fix typecheck error * VRT * Release notes * VRT * Update style * Fix budgeted * VRT * VRT * Revert VRT * VRT * Fix style * Revert usePreviewTransactions * Fix error * Fix reports form submit handlers (#3462) * Fix form submit handlers * Release notes *
Remove some old updater code (#3468) * remove some old updater code * remove old updater logic * CSV import e2e tests (#3467) * Fix React Aria Button hover styles (#3453) * Fiox hover styles and use className instead of inline to prepare for future css migration * Release notes * Cleanup * Update edit rule hover style * Undoable auto transfer notes + auto notes for cover (#3411) * Undo auto transfer notes + auto notes for cover * Release notes * Fix notes * Fix notes undo * Do not show clicked category on transfer or cover menus * Fix typecheck error * typecheck * Fix removeCategoriesFromGroups * 🐛 (reports) deleting custom report should remove it from the dashboard (#3469) * Revert "CSV import e2e tests (#3467)" (#3474) This reverts commit 5e12d40. * ♻️ (synced-prefs) separate metadata and local prefs out (#3458) *
Replace deprecated file protocol registration (#3475) * replace deprecated file handler in electron * release notes * types eh * types * update sql regexp to default to empty string when field is null (#3480) * ♻️ rename report/rollover budget to tracking/envelope (#3483) * 🐛 (import) fix csv import checkboxes not working (#3478) * Update tooltip and themes with better visibility (#3298) * Update tooltip and themes with better visibility * Rename merge request # into release notes * rename release note * update VRT * tweak light theme * dont put border on autocomplete menus * update VRT * tweak popover style * simplify * update VRT * update VRT --------- Co-authored-by: Dustin Conlon <dustin@dustinconlon.com> Co-authored-by: Dustin Conlon <58367364+VoltaicGRiD@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * fix modals on mobile BudgetTable (#3487) * Fix privacy filter (#3472) * Fix privacy filter * Release notes * Coderabbit suggestion * VRT * VRT * Revert VRT * VRT * VRT * VRT * VRT * Delete VRT * VRT * Revert VRT * 🐛 fix custom reports crashing when opening table (#3484) * 🧪 (tests) adding custom report e2e tests (#3493) * ✨ (dashboards) ability to save filters & timeframes on spending widgets (#3432) * marked files for translation * Revert ":bug: fix account filters being overridden (#3441)" This reverts commit 7336bad. * Revert "Revert ":bug: fix account filters being overridden (#3441)"" This reverts commit 5cbadc4. * Revert changes due to failed rebase * removed import of t * fixed lint warning * added PayeeTableRow.tsx * needed changes * bugfix: pluralization * variables adjustments * removed doubled trans --------- Co-authored-by: Matiss Janis Aboltins <matiss@mja.lv> Co-authored-by: Michael Clark <5285928+MikesGlitch@users.noreply.github.com> Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> Co-authored-by: Koen van Staveren <koenvanstaveren@hotmail.com> Co-authored-by: Ryan Bianchi <1435081+qedi-r@users.noreply.github.com> Co-authored-by: Robert Dyer <rdyer@unl.edu> Co-authored-by: Dustin Conlon <dustin@dustinconlon.com> Co-authored-by: Dustin Conlon <58367364+VoltaicGRiD@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> Co-authored-by: Tim <hello@timsmart.co>
Fixes regression from #3483