Split payee on mobile #2989
Split payee on mobile #2989
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 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
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Unfortunately this doesn't flow well in mobile as it turns splits in to mystery transactions. Edge: This PR: In hindsight this is the same as the recent change desktop's collapsed split: I don't think turning transactions in to mysteries is a good thing, is there some way to show the first payee(or payee with the largest share) of the split as a default? Some mock ups - excuse my MS Paint skills.
My preference would be the last one, the count would only show if there were more than 1 unique payees as it'd be redundant to have a count where the payee is the same but the category differs. This would also be an enhancement over the current state. Similar could be done for desktop collapsed. |
|
I like that idea. We could possibly implement that in desktop too if a split transaction is collapsed. |
|
I like @Teprifer 's first and last mockups. |
|
Just curious, but if there is a 'Split' (and icon) right below, do we need to say 'split' twice? |
I agree, maybe one idea is moving the "split" icon up to the payee, as it is on desktop, instead, but removing the "Split" text so it's similar to what I proposed here? |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
@joel-jeremy is this ready for review? |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
WalkthroughThe pull request introduces modifications to two components: 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/transactions/Transaction.jsx (2)
121-127: LGTM! Consider destructuring for improved readability.The addition of conditional styling for the
Viewcomponent is a good enhancement. It allows for dynamic coloring based on the transaction's state, which aligns with the PR's objective of improving the mobile interface for split payees.For improved readability, consider destructuring
textStyleat the beginning of the component:const { color: textStyleColor } = textStyle;Then use it in the style object:
color: textStyleColor || theme.tableText,This would make the code slightly more concise and easier to read.
143-151: LGTM! Consider optimizing the conditional styling.The additional styling for empty or 'Split' descriptions enhances the visual clarity of the transaction list, addressing the concerns raised in the PR comments about the clarity of split transactions.
To optimize the code and reduce repetition, consider combining the conditions:
...(!isPreview && (prettyDescription === '' || prettyDescription === 'Split') && { color: theme.tableTextSubdued, fontStyle: 'italic', }),This change would make the code more concise and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-individual-account-page-and-checks-that-filtering-is-working-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-individual-account-page-and-checks-that-filtering-is-working-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-individual-account-page-and-checks-that-filtering-is-working-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/2989.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (3 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (3)
154-154: LGTM! Improved clarity for transactions without payees.Changing the default text from 'Empty' to 'No payee' enhances the user's understanding of the transaction state. This modification aligns well with the PR's objective of improving the mobile interface.
Line range hint
1-233: Overall, the changes improve the mobile interface for transactions.The modifications in this file enhance the visual representation of transactions, particularly for split transactions and those without payees. The changes address the concerns raised in the PR comments and contribute to a more intuitive and consistent user experience on mobile devices. The code quality is good, with only minor suggestions for optimization and verification.
Key improvements:
- Better visual distinction for empty and split transactions
- Clearer labeling for transactions without payees
- Improved color consistency with the overall theme
These changes align well with the PR objectives of enhancing the mobile interface for split payees.
183-183: LGTM! Verify color consistency across the app.Changing the uncleared transaction icon color to
theme.tableTextSubduedimproves consistency within the table context. This subtle change enhances the overall visual cohesion of the interface.To ensure consistency, let's verify the usage of
theme.tableTextSubduedacross the app:This script will help us verify that the color change is consistent with other uses of
theme.tableTextSubduedand that there are no remaining instances ofpageTextSubduedin this file that should also be updated.✅ Verification successful
Verification Successful: Color Consistency Maintained
The color change to
theme.tableTextSubduedinTransaction.jsxhas been correctly applied, and no instances oftheme.pageTextSubduedremain in this file. The usage oftheme.tableTextSubduedis consistent with its application throughout the app, ensuring visual cohesion.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of theme.tableTextSubdued across the app # Search for theme.tableTextSubdued in all JavaScript and TypeScript files echo "Occurrences of theme.tableTextSubdued:" rg --type js --type ts 'theme\.tableTextSubdued' -C 2 # Compare with occurrences of theme.pageTextSubdued echo "\nOccurrences of theme.pageTextSubdued:" rg --type js --type ts 'theme\.pageTextSubdued' -C 2 # Check if there are any remaining occurrences of pageTextSubdued in this file echo "\nRemaining occurrences of pageTextSubdued in Transaction.jsx:" rg --type js 'pageTextSubdued' packages/desktop-client/src/components/mobile/transactions/Transaction.jsxLength of output: 25479
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (2)
72-76: LGTM!The
getDescriptionPrettyfunction now correctly handles parent transactions by checking theis_parentproperty and returning 'Split' when applicable. This ensures split transactions are properly labeled in the UI.
Line range hint
228-245: LGTM!The added conditional rendering in the
Footercomponent displays a 'Select account' button when a transaction lacks an associated account. This improvement enhances user experience by prompting users to select an account before proceeding.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
No description provided.