[Mobile]: More functionalities #2472
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
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
|
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
16e2a9b to
c39b63d
Compare
joel-jeremy
changed the title
|
This is a massive amount of LOC. I started reviewing, but quickly got lost. Any chance you could break this out in smaller PRs? |
Sorry for the big PR. I had some extra time so I tried doing as much as I can before I get a bit busy again. Any tips how to break this down into smaller chunks? For some of the changes, I have:
|
| @@ -281,17 +335,22 @@ export function Modals() { | |||
|
|
|||
| case 'rollover-budget-summary': | |||
| return ( | |||
| <RolloverBudgetSummary | |||
| <NamespaceContext.Provider | |||
There was a problem hiding this comment.
We need to think about how to remove the need to wrap modal components with this in order for useSheetValue to work. By default, there is no namespace context for any of our modals.
This list is a great starting point for the breaking-up into smaller chunks. Especially the bigger changes such as file/component moves (with no logic changes) could be extracted to separate PRs for a big drop in LOC here. |
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
f210346 to
14dc0cd
Compare
|
@MatissJanis I've split the autocomplete changes here: #2500 |
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
14dc0cd to
2cfdee9
Compare
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
Show resolved
Hide resolved
| @@ -0,0 +1,44 @@ | |||
| import React, { type ComponentPropsWithoutRef } from 'react'; | |||
There was a problem hiding this comment.
This was extracted from the BalanceTooltip component
| @@ -0,0 +1,67 @@ | |||
| import React, { type ComponentPropsWithoutRef } from 'react'; | |||
There was a problem hiding this comment.
This was extracted from the BalanceTooltip component
| function CreatePayeeIcon(props) { | ||
| return <SvgAdd {...props} width={14} height={14} />; | ||
| } | ||
| import { AccountAutocompleteModal } from './AccountAutocompleteModal'; |
There was a problem hiding this comment.
Moved the autocomplete components here to their own files sot they can be reused.
| @@ -0,0 +1,53 @@ | |||
| import React, { type ComponentPropsWithoutRef } from 'react'; | |||
There was a problem hiding this comment.
This was extracted from the ToBudget component
| @@ -0,0 +1,92 @@ | |||
| import React, { type ComponentPropsWithoutRef } from 'react'; | |||
There was a problem hiding this comment.
This was extracted from the ToBudget component
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
9fba1cb to
888abe0
Compare
|
Split menu components here: #2511 |
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
ddbfee4 to
29166fc
Compare
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
29166fc to
901d0c4
Compare
joel-jeremy
force-pushed
the
mobile-menu-modal
branch
from
901d0c4 to
db44102
Compare
|
@MatissJanis This should now be ready for review :) Most related refactors have been merged to master. |
| @@ -166,6 +166,7 @@ type AccountItemProps = { | |||
| embedded?: boolean; | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line import/no-unused-modules | |||
There was a problem hiding this comment.
❓ question: what's the plan with this? Do you plan on using it in the near future? If not - we can remove the export statement (which should solve the eslint issue).
Same question for the other files.
| return db.insertPayee({ name }); | ||
| const id = await db.insertPayee({ name }); | ||
| return id; |
There was a problem hiding this comment.
🥜 nitpick: unnecessary change
upcoming-release-notes/2472.md
Outdated
| authors: [joel-jeremy] | ||
| --- | ||
|
|
||
| Mobile menu modals. |
There was a problem hiding this comment.
💬 suggestion: a longer description would be better. There are folks that actually read the changelog :)
| @@ -0,0 +1,220 @@ | |||
| // @ts-strict-ignore | |||
There was a problem hiding this comment.
💬 suggestion: anything we could do here? Ideally new files should be strict-mode compliant from the get-go.
|
|
||
| const category = categories.find(c => c.id === categoryId); | ||
|
|
||
| if (category == null) { |
There was a problem hiding this comment.
🥜 nitpick:
| if (category == null) { | |
| if (category === null) { |
trafico-bot
bot
added
✨ Merged
Added for mobile: