Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mobile transaction form requires additional click to unfocus amount input #3274

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

minajevs
Copy link

Closes #3268

So the "bug" happened because setTimeout in onBlur handler messed up event-loop. The current edited field was reset on the next cycle, after onClick handler of the next field was fired. Removing setTimeout linearizes event-loop and current edited field is reset before onClick is fired.

@actual-github-bot actual-github-bot bot changed the title Fix mobile transaction form requires additional click to unfocus amount input [WIP] Fix mobile transaction form requires additional click to unfocus amount input Aug 16, 2024
Copy link

netlify bot commented Aug 16, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 2b6c9b1
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/6746ebb6448c0a0008748a9d
😎 Deploy Preview https://deploy-preview-3274.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Aug 16, 2024

Bundle Stats — desktop-client

Hey 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

Files count Total bundle size % Changed
10 5.45 MB → 5.45 MB (-4.69 kB) -0.08%
Changeset
File Δ Size
src/components/mobile/transactions/FocusableAmountInput.tsx 📈 +240 B (+3.99%) 5.88 kB → 6.11 kB
src/components/mobile/transactions/TransactionEdit.jsx 📉 -2.89 kB (-8.52%) 33.88 kB → 30.99 kB
src/components/modals/EnvelopeBudgetMenuModal.tsx 📉 -253 B (-9.22%) 2.68 kB → 2.43 kB
src/components/modals/TrackingBudgetMenuModal.tsx 📉 -253 B (-9.23%) 2.68 kB → 2.43 kB
src/hooks/useSingleActiveEditForm.tsx 🔥 -1.54 kB (-100%) 1.54 kB → 0 B
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

Asset File Size % Changed
static/js/index.js 3.44 MB → 3.43 MB (-4.69 kB) -0.13%

Unchanged

Asset File Size % Changed
static/js/workbox-window.prod.es5.js 5.69 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/useAccountPreviewTransactions.js 1.68 kB 0%
static/js/narrow.js 82.93 kB 0%
static/js/AppliedFilters.js 21.32 kB 0%
static/js/wide.js 241.19 kB 0%
static/js/ReportRouter.js 1.52 MB 0%

Copy link
Contributor

github-actions bot commented Aug 16, 2024

Bundle Stats — loot-core

Hey 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

Files count Total bundle size % Changed
1 1.32 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.32 MB 0%

@minajevs minajevs marked this pull request as draft August 18, 2024 08:54
@minajevs minajevs marked this pull request as ready for review August 18, 2024 08:54
@actual-github-bot actual-github-bot bot changed the title [WIP] Fix mobile transaction form requires additional click to unfocus amount input Fix mobile transaction form requires additional click to unfocus amount input Aug 18, 2024
@MatissJanis MatissJanis added this to the v24.9.0 milestone Aug 22, 2024
@MatissJanis
Copy link
Member

cc @joel-jeremy would you mind taking a look at this as our mobile expert? :)

@joel-jeremy
Copy link
Contributor

This behavior was intentional and the changes here will break the hook that enforces this behavior (atleast on Chrome based browsers). The point of the hook is to only have one field being active/edited at any given time. If you click on another field while another one is active, it should not activate the clicked field but instead deactivate the current active field.

I'm open to ideas on how to handle forms on mobile but this is what I was able to come up with so far. If we are able to remove the need for this hook while having a better UX then I'm all for it. :)

@minajevs
Copy link
Author

If you click on another field while another one is active, it should not activate the clicked field but instead deactivate the current active field.

But what's the point of that behaviour? For me it is annoying because requires doubletap between the fields. I don't think I have seen such thing in any app.

@joel-jeremy
Copy link
Contributor

If you click on another field while another one is active, it should not activate the clicked field but instead deactivate the current active field.

But what's the point of that behaviour? For me it is annoying because requires doubletap between the fields. I don't think I have seen such thing in any app.

For context - that was introduced because previously the mobile budget page was all edited inline. Which means you cant just easily click out of the currently edited field e.g. Budgeted to save it without clicking on another field e.g. Spent.

Today, it's not much of an issue since the budget page fields have all been migrated to open up modals. With that, I'm okay with removing the hook altogether since the mobile app's design has now migrated away from needing this functionality.

@minajevs
Copy link
Author

If you click on another field while another one is active, it should not activate the clicked field but instead deactivate the current active field.

But what's the point of that behaviour? For me it is annoying because requires doubletap between the fields. I don't think I have seen such thing in any app.

For context - that was introduced because previously the mobile budget page was all edited inline. Which means you cant just easily click out of the currently edited field e.g. Budgeted to save it without clicking on another field e.g. Spent.

Today, it's not much of an issue since the budget page fields have all been migrated to open up modals. With that, I'm okay with removing the hook altogether since the mobile app's design has now migrated away from needing this functionality.

That makes sense, thank you. I will then go ahead and remove that hook altogether.

@MatissJanis MatissJanis removed this from the v24.9.0 milestone Aug 28, 2024
@minajevs
Copy link
Author

@joel-jeremy removed useSingleActiveEditForm hook and cleaned up a little bit

@joel-jeremy
Copy link
Contributor

@joel-jeremy removed useSingleActiveEditForm hook and cleaned up a little bit

Thanks! Now that the blur logic has changed, I now see a bug where if you create a split transaction, set an amount, click on another field e.g. category, the amount does not get set properly. We may need to start using onChange here instead of onBlur to set the values.

@minajevs
Copy link
Author

minajevs commented Sep 6, 2024

@joel-jeremy removed useSingleActiveEditForm hook and cleaned up a little bit

Thanks! Now that the blur logic has changed, I now see a bug where if you create a split transaction, set an amount, click on another field e.g. category, the amount does not get set properly. We may need to start using onChange here instead of onBlur to set the values.

Either I can't reproduce it from my end, or I don't get what's the issue. Do you have more detailed "how to reproduce"? :)

@MikesGlitch
Copy link
Contributor

@coderabbitai full review

Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

Walkthrough

The pull request includes significant changes to several components in the desktop client, focusing on simplifying state management related to focus handling in forms. The FocusableAmountInput component's props have been updated to enhance internal focus state management. The TransactionEdit component has removed unnecessary logic for managing active edits, leading to a more straightforward editing process. Additionally, the ReportBudgetMenuModal and RolloverBudgetMenuModal components have eliminated redundant state management related to focus, streamlining their functionality.

Changes

File Change Summary
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx Updated focused prop to defaultFocused, modified internal focus handling and negative state logic.
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx Removed SingleActiveEditFormProvider and related logic, simplifying editing field management.
packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx Removed amountFocused state and related logic, setting defaultFocused to true.
packages/desktop-client/src/components/modals/RolloverBudgetMenuModal.tsx Removed amountFocused state and related logic, setting defaultFocused to true.
packages/desktop-client/src/hooks/useSingleActiveEditForm.tsx Deleted file that managed single active edit form context and logic.

Assessment against linked issues

Objective Addressed Explanation
Clicking on form fields should switch focus immediately (issue #3268)

Possibly related PRs

Suggested labels

:sparkles: Merged

Suggested reviewers

  • joel-jeremy
  • MatissJanis

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1750cd9 and b32ebdf.

⛔ Files ignored due to path filters (1)
  • upcoming-release-notes/3274.md is excluded by !**/*.md
📒 Files selected for processing (5)
  • packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (4 hunks)
  • packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (6 hunks)
  • packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx (2 hunks)
  • packages/desktop-client/src/components/modals/RolloverBudgetMenuModal.tsx (2 hunks)
  • packages/desktop-client/src/hooks/useSingleActiveEditForm.tsx (0 hunks)
💤 Files with no reviewable changes (1)
  • packages/desktop-client/src/hooks/useSingleActiveEditForm.tsx
🔇 Additional comments (9)
packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx (1)

79-79: Approve change with clarification request

The addition of defaultFocused={true} to the FocusableAmountInput component aligns well with the PR objectives. It addresses the issue of requiring an additional click to focus on the amount input and simplifies focus management.

However, there are a couple of points that need clarification:

  1. The AI summary mentions the removal of amountFocused state and associated logic, but these changes are not visible in the provided code. Could you please confirm if these changes were made and if so, provide the updated code?

  2. While setting defaultFocused={true} solves the immediate issue, are there any potential side effects of always having the amount input focused by default? For example, could this interfere with keyboard navigation or accessibility features?

To verify the removal of amountFocused state and associated logic, please run the following script:

packages/desktop-client/src/components/modals/RolloverBudgetMenuModal.tsx (2)

81-81: Approve change with suggestions for further verification

The addition of defaultFocused={true} to the FocusableAmountInput component aligns well with the PR objectives. It simplifies focus management and potentially improves the user experience by reducing the number of clicks required to interact with the form.

To ensure the change meets the intended goals:

  1. Please verify this behavior on various mobile devices to confirm it resolves the double-tap issue mentioned in the PR objectives.
  2. Consider any potential side effects this change might have on the overall form interaction, especially in light of the previous concerns about maintaining only one active field at a time.
✅ Verification successful

Change verified and approved

The addition of defaultFocused={true} to the FocusableAmountInput component is consistently applied across the codebase, ensuring uniform focus management and enhancing the user experience as intended by the PR objectives.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any other instances of FocusableAmountInput that might need similar changes

# Test: Search for other usages of FocusableAmountInput
rg --type typescript 'FocusableAmountInput'

Length of output: 243


Script:

#!/bin/bash
# Description: Search for all instances of FocusableAmountInput in TypeScript and TSX files

# Search for 'FocusableAmountInput' in .ts and .tsx files
rg 'FocusableAmountInput' --glob '*.ts' --glob '*.tsx'

Length of output: 1010


Script:

#!/bin/bash
# Description: Inspect FocusableAmountInput usage in ReportBudgetMenuModal.tsx for focus management

# Search for props passed to FocusableAmountInput in ReportBudgetMenuModal.tsx
rg 'FocusableAmountInput\s*\(' packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx --multiline

Length of output: 116


Script:

#!/bin/bash
# Description: Extract props passed to FocusableAmountInput in ReportBudgetMenuModal.tsx

# Search for all instances of FocusableAmountInput in JSX and display the props
rg '<FocusableAmountInput\s[^>]*>' packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx --multiline

Length of output: 656


Line range hint 1-105: Overall simplification looks good, but warrants careful testing

The removal of explicit focus management using useState and useEffect hooks (as mentioned in the AI summary) simplifies the component and aligns with the PR objectives. This change, combined with the addition of defaultFocused={true}, should improve the user experience.

However, given the extent of the changes and the concerns raised in the PR comments, I recommend:

  1. Thorough testing of all form interactions, especially for split transactions. The PR comments mention a potential issue with setting amounts in split transactions, which should be carefully verified.
  2. Monitoring for any regressions related to focus management across different scenarios and device types.
  3. Verifying that the removal of the useSingleActiveEditForm hook (mentioned in PR comments) doesn't introduce unexpected behaviors in other parts of the application.

Consider documenting the rationale behind this simplification in the component or in a separate architecture document. This will help future maintainers understand the design decisions and the trade-offs made.

packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1)

162-162: ⚠️ Potential issue

Renaming 'focused' prop to 'defaultFocused' may impact existing component usage

Changing the prop name from focused to defaultFocused modifies the public API of the FocusableAmountInput component. This can affect other parts of the codebase that rely on the focused prop. Please ensure that all instances where FocusableAmountInput is used with the focused prop are updated to prevent any unexpected behavior.

Run the following script to identify usages of the old focused prop:

packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (5)

349-349: Consistent Disabling of Category Field

The addition of disabled={isOffBudget || isBudgetTransfer(transaction)} on line 349 ensures that the category field is disabled when the transaction is off-budget or a budget transfer. This maintains consistency with the intended behavior and prevents unintended category changes.


679-679: Enhance User Experience by Focusing Amount Input

Setting defaultFocused={true} on the FocusableAmountInput component ensures that the amount field is focused when the page loads. This improves the user experience by allowing immediate data entry without additional clicks.


719-719: Disable Category Field for Off-Budget and Transfer Transactions

Adding disabled={isOffBudget || isBudgetTransfer(transaction)} on line 719 aligns the parent transaction's category field behavior with the child transactions. This prevents editing the category when it's not applicable, maintaining consistency across the form.


783-783: Verify Disabling Account Change on Existing Transactions

The account field is now disabled for existing transactions (disabled={!adding}) on line 783. While this might prevent unintended changes, it also restricts users from correcting mistakes in the account selection.

Please confirm that preventing account changes on existing transactions is intentional and doesn't hinder necessary edits. If users need the ability to change accounts, consider enabling this field.


1113-1120: Modernize Code with React Hooks

Replacing the connect HOC with React hooks (useSelector, useCategories, useAccounts, etc.) simplifies the component and aligns with current best practices. This refactoring enhances readability and maintainability of the code.

Copy link
Contributor

@MikesGlitch MikesGlitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minajevs Could you update the branch with master? It looks like there's some merge conflicts.

@matt-fidd matt-fidd added this to the v24.11.0 milestone Oct 14, 2024
@minajevs
Copy link
Author

Click on the payee/category after setting the amount:
1000004042.mp4

The amount will only get set if you click on the empty areas on the screen

Fixed the issue. Pushing updates onChange instead of onBlur caused additional weird issues with scrolling. Instead it will now just always push updates onBlur, without a condition. Seems to be working ok.

After merging latest master there also appeared a new issue with immediate payee/category/etc. modals, as they received stale update callbacks. Solved it with refs, so should be fine now.

@joel-jeremy @MikesGlitch can you please review it again? :)

@minajevs minajevs requested a review from MikesGlitch October 15, 2024 10:03
@youngcw youngcw removed this from the v24.11.0 milestone Oct 24, 2024
@minajevs
Copy link
Author

/update-vrt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: On a "new transaction" mobile page clicking "payee" takes two clicks instead of one
6 participants