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
WalkthroughThe pull request introduces several modifications to the Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 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 (
|
![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 tried it with larger padding and it just looks kinda weird with so much space. |
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
from
5d4c149
to
f6a5fc5
Compare
gabeklavans
force-pushed
the
mobile-multi-account-view
branch
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? |
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