[Mobile] Fix #3214 - Pull down to refresh triggering clicks on budget cells #3374
Conversation
joel-jeremy
force-pushed
the
fix-3214
branch
from
c7d9072 to
58f58a0
Compare
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ 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
|
| @@ -53,7 +53,7 @@ export function usePreviewTransactions( | |||
| })), | |||
| })); | |||
| setPreviewTransactions(ungroupTransactions(withDefaults)); | |||
| collapseTransactions(withDefaults.map(t => t.id)); | |||
| collapseTransactions?.(withDefaults.map(t => t.id)); | |||
There was a problem hiding this comment.
This is resulting in an error in console.log. Fixing it here.
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
fix-3214
branch
from
6af1fee to
ff6f7c8
Compare
There was a problem hiding this comment.
The sign on the budgeted value in the table disappeared
There was a problem hiding this comment.
Should be fixed now :)
There was a problem hiding this comment.
The budgeted value shifted slightly but thats not a huge deal I would think.
There was a problem hiding this comment.
Should be fixed now :)
There was a problem hiding this comment.
Still seeing the same thing. I guess it would be more accurate to say that the budgeted total gained a sign where it should be a positive listing where the header now shows negative.
There was a problem hiding this comment.
Weird. It's fixed when you access the link but it still shows in the VRT screenshot. There seem to be something weird with the VRT tests it's not detecting the changes made I had to delete the screenshots and rerun the VRT.
There was a problem hiding this comment.
This one doesn't look quite right
|
Not sure if intentional, but this changes the colour of zero values on the mobile budget table. |
joel-jeremy
force-pushed
the
fix-3214
branch
from
953d62c to
844e1bc
Compare
|
Fixed the VRT. I had to checkout the entire e2e/ folder from master and rerun the VRT again. |
I think the zero values in edge is broken. All zeroes used to be grayed out before, similar to how it is in desktop. |
Ah right. I think we might need to bump the contrast on the gray in another PR, it is quite hard to see the text - even if it is demoted in importance. |
joel-jeremy
force-pushed
the
fix-3214
branch
from
2a51e8c to
16bd509
Compare
joel-jeremy
force-pushed
the
fix-3214
branch
from
da8edec to
4cdc658
Compare
|
@MatissJanis Would you mind reviewing on top of @matt-fidd's approval? |
|
👋 I'm not sure I follow the refactored logic here. Am I missing something? The issue appeared when we moved from the old This makes me think that the patch should be in But again: probably I'm missing something. |
The issue happens because the CellValue wraps the Button2 components (budget cell and balance cell) with a Text component. The click handler is attached to the Text component instead of the underlying Button2 component which causes the quirky behavior we see here. So the refactor is to allow us to render the cell value as Button2 (without being wrapped by a Text component) and allows us to attach the click handler on the button which handles it in a more standard way. It doesn't happen in the other parts of the app because the Button2 in those parts are not wrapped with another component. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant architectural changes across multiple components in the desktop and mobile applications. It primarily focuses on enhancing the rendering of financial values by implementing a render prop pattern and introducing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
…ks on budget cells (actualbudget#3374) * Fix actualbudget#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
* 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) *
Resolves #3214
The
CellValuecomponent has been refactored to be more composable. To fix the cell value components in the mobile budget table, I had to update them to use the new react aria Button - this looks to have fixed the issue.Not sure what are causing some of VRT screenshots to change but it looks like the color changed on them.