add buttons for combined-account transaction pages on mobile #3734
Conversation
actual-github-bot
bot
changed the title
✅ 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
|
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
2632df9 to
53d7f7f
Compare
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
53d7f7f to
96e1ef5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
gabeklavans
changed the title
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
96e1ef5 to
34c8971
Compare
gabeklavans
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/mobile/accounts/Accounts.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request modifies the Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2)
Line range hint
23-53: LGTM! Consider adding accessibility labels.The transformation from View to Button with navigation functionality aligns well with the PR objectives. The styling is consistent with mobile patterns.
Consider adding an
accessibilityLabelprop to the Button for better screen reader support:<Button onPress={() => navigate(`/accounts/${id}`)} + accessibilityLabel={`View transactions for ${name}`} style={{ flex: 1,
209-220: Consider using View component instead of div for consistency.While the divider implementation is good, using a
divelement seems inconsistent with the rest of the codebase which uses theViewcomponent.Consider this change for better consistency:
- <div + <View style={{ width: '100%', height: 2, background: theme.pageText, paddingLeft: 10, paddingRight: 10, marginTop: 20, }} - /> + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3734.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/desktop-client/src/components/accounts/Account.tsx:655-665 Timestamp: 2024-10-24T17:05:41.415Z Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (3)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (3)
190-194: LGTM! Clear implementation of combined budgeted accounts view.The implementation provides a clear entry point to view all budgeted account transactions, which directly addresses the feature request from issue #2333.
224-227: LGTM! Consistent implementation for off-budget accounts view.The implementation maintains consistency with the budgeted accounts section while providing clear navigation to combined off-budget transactions.
Line range hint
23-227: Verify navigation paths for combined account views.The implementation looks solid overall and addresses the requirements from issue #2333. Let's verify the navigation paths to ensure consistency.
✅ Verification successful
Navigation implementation for combined account views is consistent
The verification shows that:
- The navigation pattern in
Accounts.jsxaligns with existing account navigation patterns in other components- Route handling for budgeted/offbudget accounts is properly defined in the sidebar component
- The
AccountHeadercomponent is consistently used across both mobile and desktop account views- The implementation maintains compatibility with existing account-related components and test files
The changes successfully implement the combined account view navigation without introducing inconsistencies or regressions in the navigation paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify navigation paths and route handlers for combined account views # Test: Check for any other components that might need similar navigation updates rg -l "navigate\(\'/accounts/" --type jsx # Test: Verify consistent route handling across the application rg -l "\/accounts\/(budgeted|offbudget)" --type jsxLength of output: 298
Script:
#!/bin/bash # Let's fix the regex syntax and try again # Check for navigation patterns to accounts rg -l "navigate\('/accounts" --type jsx # Check for route handlers with combined account paths rg -l "/accounts/(budgeted|offbudget)" --type jsx # Look for other potential route definitions rg -l "path.*accounts" --type jsx # Check for any AccountHeader usage rg -l "AccountHeader" --type jsxLength of output: 277
Script:
#!/bin/bash # Search without file type restriction since jsx type isn't recognized # Check for navigation patterns to accounts rg -l "navigate\\('/accounts" # Check for route handlers with combined account paths rg -l "/accounts/(budgeted|offbudget)" # Look for other potential route definitions rg -l "path.*accounts" # Check for any AccountHeader usage rg -l "AccountHeader" # Find all potential account-related components fd -e jsx -e js | grep -i "account"Length of output: 1595
Fair. I can try adding another button on top of all the current ones for All Accounts, using the same header component for consistency. |
|
Thanks for this! I can verify that the functionality if good but the UI definitely needs some work. How about making the For Budget and Off Budget buttons more like headers and just put a cheveron to indicate that they are clickable? |
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
34c8971 to
edc046e
Compare
Yup, I like the sound of that. How's this next iteration I just pushed look? Also, I may forgo adding the "All Accounts" view for this PR. It seems to be quite a bit more work to get right compared to the small change I'm making here, but this PR already solves the issue like 90% of the way for me, if that makes sense. |
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
edc046e to
6869ef0
Compare
|
I think this style with the chevrons looks really good. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2)
23-62: Good implementation of interactive headers with visual indicators.
The conversion to Button component and addition of chevron icon successfully implements the reviewer's suggestions and improves mobile UX.
However, here are some suggested improvements:
- Add ARIA label for better accessibility:
<Button
variant="bare"
+ aria-label={`View ${name} transactions`}
onPress={() => navigate(`/accounts/${id}`)}
style={{- Consider adding hover/active states for better interaction feedback:
<Button
variant="bare"
onPress={() => navigate(`/accounts/${id}`)}
style={{
flex: 1,
flexDirection: 'row',
marginTop: 10,
padding: 0,
color: theme.pageTextLight,
width: '100%',
+ ':hover': {
+ backgroundColor: theme.hover
+ },
+ ':active': {
+ backgroundColor: theme.buttonBackgroundActive
+ },
...style,
}}
>199-203: Current implementation provides a good foundation for mobile account navigation.
The addition of combined views for budgeted and off-budget accounts improves mobile navigation. However, to fully address issue #2333, consider these future enhancements:
- Add an "All Accounts" view as discussed in the PR comments
- Consider adding a loading state for the navigation transitions
For the "All Accounts" view, you could add it as:
<AccountHeader
id="all"
name="All Accounts"
amount={getOnBudgetBalance() + getOffBudgetBalance()}
style={{ marginBottom: 20 }}
/>Also applies to: 220-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3734.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2)
11-11: LGTM: Import addition aligns with UI requirements.
The addition of SvgCheveronRight import supports the reviewer's suggestion to add visual indicators for clickable headers.
27-57: Investigate visual regression test failures.
The Button component and chevron icon additions could affect layout in ways that trigger visual regression tests. Let's verify the impact:
|
I agree, looks good. |
|
Looking better! Just one more thing, I think the new buttons could use some padding so that the touch target is larger. Also you need to update the VRT screenshots. Please see https://github.com/actualbudget/actual/tree/master/packages/desktop-client#visual-regression |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
27-42: Consider increasing touch target size for better mobile accessibility.
The current padding (7px top/bottom) might be insufficient for comfortable touch interaction on mobile devices. Apple's Human Interface Guidelines recommend a minimum touch target size of 44x44 points.
<Button
variant="bare"
aria-label={`View ${name} transactions`}
onPress={() => navigate(`/accounts/${id}`)}
style={{
flex: 1,
flexDirection: 'row',
- paddingTop: 7,
- paddingBottom: 7,
+ paddingTop: 12,
+ paddingBottom: 12,
paddingLeft: 0,
paddingRight: 0,
color: theme.pageTextLight,
width: '100%',
...style,
}}
>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2)
23-65: Update visual regression test screenshots.
The UI changes (new chevron icon, button styling, and section margins) will require updates to the visual regression test screenshots. This aligns with the VRT discrepancies mentioned in the PR comments.
#!/bin/bash
# Description: Check for existing visual regression tests
# Test: Look for VRT screenshot references
rg -l "accounts.*\.(png|jpg|jpeg)" --glob '!{node_modules,build,dist}/**'202-206: Consider adding "All Accounts" view as discussed.
Based on the PR objectives and issue #2333, an "All Accounts" view was considered to show combined transactions. While the current implementation adds navigation for budgeted and off-budget accounts, it might be worth implementing the "All Accounts" view in a follow-up PR.
Let's verify the route handling for these new navigation targets:
Also applies to: 223-226
I find it still a bit too small. Can we make it similar size and padding as the other account buttons, but keeping the current background? |
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
2 times, most recently
from
f6a5fc5 to
1498ae9
Compare
How's this iteration look? Also just to note, even before this iteration, these buttons I think were still larger than the ones on the budget mobile page for tapping on each category to open their notes/options. |
Looking good! One last comment: can we change the header buttons to a normal variant so that it also has the bounce effect when clicked (similar to the other account buttons) but remove the press highlight? |
Sorry for the delay, just getting back to this. So, if we're making it a normal variant again (how it was when I first opened the PR), is there a need for the chevron anymore? Since it will be pretty clear that this is a button and is clickable. If there isn't a need, how do you think we should differentiate this button from the rest? A different colored border, perhaps? A different text color? Different height for the button? |
Sorry for the confusion, what I meant was to keep the current style that but remove the press highlight and add the bounce effect that the normal variant has. This is so that all the buttons still have the same look and feel. Press highlight: Actual.-.Google.Chrome.2024-12-05.15-46-50.mp4Mini bounce effect on click (this is enabled by the Button's Actual.-.Google.Chrome.2024-12-05.15-51-30.mp4 |
EDIT: I actually got both the highlight gone and the bounce added in one go with the workaround I mention below. It's actually not that hacky. Just required copy-pasting the button-bounce values, so could be prone to going out of style in the future. To fix that, I might take the contents of Old comment: So, I was able to refer to other places in the codebase to get rid of the press highlight. But it seems pretty hacky to get the button bounce in place for a actual/packages/desktop-client/src/components/common/Button2.tsx Lines 113 to 131 in 832fd1e All that means is we can be pretty sure that any other buttons that look like this won't bounce. I can still try to work around this if you think the effect will be worth it here. |
to match the feel of the other account buttons
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
from
1498ae9 to
19d2487
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (2)
5-5: Prefer consistent import ordering.
Although this new import looks good, consider aligning it with your existing import conventions (e.g., grouping external libraries together or alphabetically). This makes the import section more readable.
59-67: Helpful pressed/hover states.
The custom class styling ensures visual feedback, but confirm that translating by 1px downward still feels natural on mobile.You could test if a slightly larger or smaller translation offers a better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (13)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (13)
15-15: Great addition ofSvgCheveronRight.
This icon nicely communicates navigation flows, especially on mobile.
29-29: Complete and clear interface extension.
Declaringid: stringin the prop type clarifies the identity of the header for routing.
41-41: Straightforward usage ofuseNavigate.
This hook is suitable for mobile navigation flows and helps maintain consistent code with the rest of the app.
44-47: Button usage is solid, but consider consistent accessibility labeling.
Thearia-labelhelps screen readers. Ensure that the label matches the user-facing text or otherwise clarifies action and context for accessibility.
51-54: Appropriate padding for touch targets.
The chosen top/bottom padding of 15px is typically good for mobile. Ensure any sibling components match or exceed these dimensions for consistent touch areas.
69-69: Container layout is well-structured.
This flex layout with centered alignment ensures the text and icon are well-positioned. No issues spotted here.
73-73: Font size of 17px is typical on mobile.
This size aligns with accessible design guidelines, but ensure consistent usage across the rest of the app’s mobile components.
93-93: Consistent text styling.
Reusingstyles.textis good for a uniform UI. Ensure that the rest of the cell value styling is also consistent elsewhere in the code to avoid style drift.
97-97: Closing button tag structure.
The new layout is clean. Watch out for any possible onPress conflicts if the nested children also have touch handlers.
259-263: Enhanced navigability for On Budget section.
Introducing anid="onbudget"clarifies the route. Confirm that users understand the label "On budget" vs. "Off budget."
280-280: Similarly, off-budget section is consistent with the “On budget” approach.
This symmetry helps users stay oriented across different account groups on mobile.
36-36: Ensureidis mandatory at call sites.
Usingidin multiple places makes sense, but double check that all existingAccountHeaderusage indeed passes anidto avoid runtime errors.✅ Verification successful
Let me check the
AccountHeadercomponent definition to understand its props interface.
Let me try a different approach to find the component definition.
All
AccountHeaderusages properly handle theidprop requirementBased on the verification results, there are three different
AccountHeadercomponents in different files, each with its own prop interface:
In
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx:
- All usages pass the required
idprop directly (onbudgetandoffbudget)- The component's type definition explicitly requires
id: stringIn
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx:
- Passes the full
accountobject which contains theidfield (verified from the code usingaccount.id)In
packages/desktop-client/src/components/accounts/Header.tsx:
- Different component with different props interface
- Receives the full
accountobject which contains theidfield🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all <AccountHeader> usage includes 'id' prop rg -A 3 '<AccountHeader'Length of output: 1696
Script:
#!/bin/bash # Find AccountHeader component definition ast-grep --pattern 'interface AccountHeaderProps { $$$ }' # Also search for type definition ast-grep --pattern 'type AccountHeaderProps = { $$$ }'Length of output: 120
Script:
#!/bin/bash # Find AccountHeader component and its props rg -A 10 "AccountHeader.*Props" # Also search for the component definition directly rg -A 15 "const AccountHeader.*=|function AccountHeader"Length of output: 8774
79-87: Chevrons for clear forward navigation.
These lines effectively indicate navigability. If possible, also check that the color contrast meets WCAG accessibility guidelines.
|
This looks good to me! Thanks! |
I quickly threw together the actual styling of these buttons. I'm very much open to changing it up, but just wanted to start the functionality here.
I was pleasantly surprised to see that the combined accounts pages that work on desktop also seem to work fine on mobile. This just adds a way to navigate to those URLs on mobile.
resolves #2333