[Order - Shipping - Custom Amounts - Cash] Highlight amount when focused #14030

toupper · 2024-09-24T15:14:18Z

Description

With this PR we highlight the amount field when is focused.

The reason why this issue happens is because the FormattableAmountTextField is composed of a hidden TextField to gather the input data and a Text to show the formatted amount. Since the text field is hidden, the editing cursor is also invisible, making it a bit confusing to the user who doesn't know for sure whether they are editing it or not.

With this PR we highlight the Text when the hidden Text Field is focused. That shows the user that the amount text field is focused and they can enter the data. Ideally, we would show the cursor, but that implies a refactor into this class that might not be worth the effort. We can revisit this decision depending on user feedback.

Steps to reproduce

Go to Orders
Tap on + to create a new order
Add a product
Tap on Shipping
Tap on the Amount Text
Before it wasn't highlighted when focused, now it is.

Testing information

This component is also used in Custom Amounts and Cash Payments, we have to cover also that. (see videos)

Screenshots

Shipping

ScreenRecording_09-24-2024.17-01-28_1.MP4

Custom Amounts

ScreenRecording_09-24-2024.17-02-06_1.MP4

Cash Payments

ScreenRecording_09-24-2024.17-03-47_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.

wpmobilebot · 2024-09-24T15:28:17Z

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

	App Name	WooCommerce iOS
	Build Number	pr14030-f3eaa7a
	Version	20.7
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`f3eaa7a`
	App Center Build	WooCommerce - Prototype Builds #11163

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

rachelmcr

This seems like a reasonable compromise, to avoid a significant refactor!

A couple notes:

There are a couple UI tests that need to be updated to handle the changes here, see also my comment below.
Did you consider using something like a border around the text field to indicate that it has focus? That has the advantage of working even if there is an amount already entered. This approach works to highlight the empty field, but there's no change if it has a value. (I think it's fine to leave it as-is, just mentioning it as an alternate approach.)

rachelmcr · 2024-09-24T16:16:58Z

...mmerce/Classes/ViewRelated/ReusableViews/SwiftUI Components/FormattableAmountTextField.swift

@@ -17,7 +17,9 @@ struct FormattableAmountTextField: View {
 // Hidden input text field
 TextField("", text: $viewModel.textFieldAmountText)
 .onChange(of: viewModel.textFieldAmountText, perform: viewModel.updateAmount)
- .focused()


Are you intentionally removing the auto-focus this field gets from the .focused() modifier?

If so, we'll need to update the UI tests to tap on this field so it gets focused. (Right now, the tests that include this field are failing because the field doesn't get focus so the keyboard doesn't appear to enter the amount.)

Alternately, we could keep the .focused() modifier and just add the .onChange(of: focusAmountInput) modifier on top of it. That way the field still gets auto-focus but the placeholder text is also highlighted.

toupper · 2024-09-25T14:53:17Z

Thanks for your input @rachelmcr ! That makes sense. I tried with the border and this is how it looks:

@joe-keenan We can use your input here :) We're trying to make it visible to the user when the formattable amount text field, a component we use in several parts of the app is focused. Since this is a custom component we don't show the cursor, which makes it difficult to communicate to the user that it's being focused and they can edit it (see original issue)

What approach do you think it's more convenient and consistent?

Change the text color when focused (see description)
User border when focused (see images in this comment) I lean towards this approach, but the border could just be the text field border and be confusing. Do you have any suggestions for making it more intuitive?

rachelmcr · 2024-09-25T14:59:57Z

That border looks nice! What if you add the border modifier after the .frame(maxWidth: .infinity, alignment: .leading) modifier, so the border goes around the full width of the screen (not just the amount text)? That might have a little more of the feel of highlighting the entire field rather than just being a border around the text.

toupper · 2024-09-25T15:18:29Z

That border looks nice! What if you add the border modifier after the .frame(maxWidth: .infinity, alignment: .leading) modifier, so the border goes around the full width of the screen (not just the amount text)? That might have a little more of the feel of highlighting the entire field rather than just being a border around the text.

Good point, this is how it looks now:

joe-keenan · 2024-09-26T00:24:02Z

Hey all, thanks for the ping.

I included the cursor in all my designs for this text field style. It’s the most commonly used element of all text fields to indicate not just that the field is focused, but also where the user should expect to see their typing appear. I’ve been meaning to raise it in my design dusting posts soon.

Are you really sure we can’t show it? I think it breaks the UX not to have it. I also noticed that in your first video, for shipping, the cursor is visible.

ichqhxDjbmZZmvJaJkGhkj-fi-698_29716 (Link to Figma designs for custom amounts)

As for focusing the field, the border below the text area is supposed to be the bottom of the field, so my first thought is to try making that border bold and purple. This is probably a good idea even if we show the cursor.

wpmobilebot · 2024-09-27T11:00:08Z

Version 20.6 has now entered code-freeze, so the milestone of this PR has been updated to 20.7.

dangermattic · 2024-10-02T14:00:12Z

	1 Warning
⚠️	This PR is assigned to the milestone 20.8. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

wpmobilebot · 2024-10-11T02:50:49Z

Version 20.7 has now entered code-freeze, so the milestone of this PR has been updated to 20.8.

This reverts commit 3601b15.

…able text field

toupper · 2024-10-16T11:24:28Z

@rachelmcr Could you please check the implementation again? For now I think showing the border is a good idea, and I added this issue to add the cursor eventually.

rachelmcr

Looks great! I tested the three places where this field is used, and it worked as expected everywhere:

Custom amount
Shipping amount
Cash payment amount

Thanks for opening an issue about the cursor, too; I think this behavior is a good compromise for now but it's good to keep track of that improvement.

rachelmcr · 2024-10-16T12:42:31Z

...mmerce/Classes/ViewRelated/ReusableViews/SwiftUI Components/FormattableAmountTextField.swift

@@ -28,6 +28,8 @@ struct FormattableAmountTextField: View {
 .minimumScaleFactor(0.1)
 .lineLimit(1)
 .frame(maxWidth: .infinity, alignment: .leading)
+ .padding(5)
+ .roundedBorder(cornerRadius: 8, lineColor: focusAmountInput ? Color(.wooCommercePurple(.shade60)) : .clear, lineWidth: 1)


This works great! Just to offer an alternate approach, I find that our if modifier helps clarify the intent when reading this (clarifying that we add a border if the field has focus):

.if(focusAmountInput, transform: { field in field.roundedBorder(cornerRadius: 8, lineColor: Color(.wooCommercePurple(.shade60)), lineWidth: 1) })

But no need to make any changes; just mentioning it for the potential readability.

Thanks for the review and the suggestion @rachelmcr ! Indeed, it makes it more readable. I added it in the last commit

Highlight amount when focused

06e000d

toupper added type: bug A confirmed bug. feature: order creation All tasks related to creating an order labels Sep 24, 2024

toupper added this to the 20.6 milestone Sep 24, 2024

toupper requested a review from rachelmcr September 24, 2024 15:14

Add release note

d9399c6

rachelmcr self-assigned this Sep 24, 2024

rachelmcr requested changes Sep 24, 2024

View reviewed changes

rachelmcr removed their assignment Sep 24, 2024

wpmobilebot modified the milestones: 20.6, 20.7 Sep 27, 2024

WIP: Add cursor to FormattableAmountTextField

3601b15

wpmobilebot modified the milestones: 20.7, 20.8 Oct 11, 2024

toupper added 3 commits October 16, 2024 13:09

Revert "WIP: Add cursor to FormattableAmountTextField"

420de52

This reverts commit 3601b15.

Add border instead of font color change when highlighting the formatt…

741aacb

…able text field

Merge branch 'trunk' into issue/13956-highlight-amount-field-shipping

814e498

toupper mentioned this pull request Oct 16, 2024

Add cursor to the FormattableAmountTextField #14135

Open

toupper requested a review from rachelmcr October 16, 2024 11:25

rachelmcr self-assigned this Oct 16, 2024

rachelmcr approved these changes Oct 16, 2024

View reviewed changes

rachelmcr removed their assignment Oct 16, 2024

Make modifier more readable

f3eaa7a

toupper merged commit 6c369e4 into trunk Oct 16, 2024
14 checks passed

toupper deleted the issue/13956-highlight-amount-field-shipping branch October 16, 2024 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Order - Shipping - Custom Amounts - Cash] Highlight amount when focused #14030

[Order - Shipping - Custom Amounts - Cash] Highlight amount when focused #14030

toupper commented Sep 24, 2024 •

edited by rachelmcr

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

rachelmcr left a comment

rachelmcr Sep 24, 2024

toupper commented Sep 25, 2024

rachelmcr commented Sep 25, 2024

toupper commented Sep 25, 2024

joe-keenan commented Sep 26, 2024 •

edited

Loading

wpmobilebot commented Sep 27, 2024

dangermattic commented Oct 2, 2024 •

edited

Loading

wpmobilebot commented Oct 11, 2024

toupper commented Oct 16, 2024

rachelmcr left a comment

rachelmcr Oct 16, 2024

toupper Oct 16, 2024

[Order - Shipping - Custom Amounts - Cash] Highlight amount when focused #14030

[Order - Shipping - Custom Amounts - Cash] Highlight amount when focused #14030

Conversation

toupper commented Sep 24, 2024 • edited by rachelmcr Loading

Description

Steps to reproduce

Testing information

Screenshots

Shipping

Custom Amounts

Cash Payments

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

wpmobilebot commented Sep 24, 2024 • edited Loading

rachelmcr left a comment

Choose a reason for hiding this comment

rachelmcr Sep 24, 2024

Choose a reason for hiding this comment

toupper commented Sep 25, 2024

rachelmcr commented Sep 25, 2024

toupper commented Sep 25, 2024

joe-keenan commented Sep 26, 2024 • edited Loading

wpmobilebot commented Sep 27, 2024

dangermattic commented Oct 2, 2024 • edited Loading

wpmobilebot commented Oct 11, 2024

toupper commented Oct 16, 2024

rachelmcr left a comment

Choose a reason for hiding this comment

rachelmcr Oct 16, 2024

Choose a reason for hiding this comment

toupper Oct 16, 2024

Choose a reason for hiding this comment

toupper commented Sep 24, 2024 •

edited by rachelmcr

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

joe-keenan commented Sep 26, 2024 •

edited

Loading

dangermattic commented Oct 2, 2024 •

edited

Loading