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

POS: Dismiss Cash and Email view keyboard before the view disappears #14902

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Jan 17, 2025

Merge after #14854

Description

In Cash and Email Receipt views the keyboard would disappear after the view disappears without animation which looked like a glitch.

A solution is to manually dismiss focus of keyboard when back button is tapped or when the submission is succesful.

Steps to reproduce

POS

  • Set .acceptCashForPointOfSale flag to true
  • Within a POS transaction, selecting to pay by cash, observe that the cash collection view appears under an animated transition and keyboard appears with the animation as well.
  • Tap back, confirm that the keyboard disappears with a transition.
  • Upon marking the order as complete (any or no amount), the keyboard also disappears with a transition.
  • Tapping on "send receipt" appears/disappears keyboard appears with a transition.
  • Tap back, confirm that the keyboard disappears with a transition.
  • Submit email, confirm that the keyboard disappears with a transition.

Regression

  • Create an order with Custom Amount
  • Confirm the keyboard appears in Custom Amount view
  • Dismiss the keyboard
  • Tap on the amount field again
  • Confirm the keyboard appears again

Testing information

Screenshots

ScreenRecording_01-17-2025.11-13-43_1.MP4
ScreenRecording_01-17-2025.11-06-39_1.MP4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@staskus staskus added type: enhancement A request for an enhancement. feature: POS labels Jan 17, 2025
@staskus staskus added this to the 21.5 milestone Jan 17, 2025
@staskus staskus requested a review from iamgabrielma January 17, 2025 09:26
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 17, 2025

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14902-3891f73
Version21.4
Bundle IDcom.automattic.alpha.woocommerce
Commit3891f73
App Center BuildWooCommerce - Prototype Builds #12618
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus marked this pull request as ready for review January 20, 2025 08:00
Copy link
Contributor

@iamgabrielma iamgabrielma left a comment

Choose a reason for hiding this comment

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

Works nicely, dropped a comment on a bit I'm not sure I fully understand, as seems we're not using it.

🚢

Comment on lines 54 to 58
func focused(_ focusBinding: FocusState<Bool>.Binding) -> FormattableAmountTextField {
var copy = self
copy.externalFocusBinding = focusBinding
return copy
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure to understand the usage of computed focusAmountInput with re-declaring focused(...) and differentiating internal from external focus input here (so the commit 990053d ) 🤔

We seem to use the default SwiftUI's focused<Value>(_ binding: FocusState<Value>.Binding, ... and commenting this out seems to work just fine, so we could remove these as well and be enough to set isTextFieldFocused = false as needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you're right! 💯

I didn't think the default .focused would work when passing .focused through outside and within FormattableAmountTextField at the same time but I was wrong.

@staskus staskus force-pushed the fix/pos-cash-email-view-dismiss-keyboard branch from bdb9a8d to 8734617 Compare January 21, 2025 11:38
@staskus staskus enabled auto-merge January 21, 2025 11:39
@staskus staskus merged commit 4b2bce1 into trunk Jan 21, 2025
12 checks passed
@staskus staskus deleted the fix/pos-cash-email-view-dismiss-keyboard branch January 21, 2025 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: POS type: enhancement A request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants