-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Rename BuiltInReader / LocalMobileReader references to TapToPay #15380
Conversation
Generated by 🚫 Danger |
|
a1abb8c
to
9fd3115
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well. Thanks for tidying this up, it's a pain but much simpler if we can refer to one name...
CardReader(serial: "APPLE-BUILT-IN-SIMULATOR-1", | ||
vendorIdentifier: "SIMULATOR", | ||
name: "Simulated Apple Built In Reader", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but maybe worth updating these strings too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these ones can be updated 👍
@@ -1257,7 +1257,7 @@ extension WooAnalyticsEvent { | |||
|
|||
enum CardReaderType: String { | |||
case external | |||
case builtIn = "built_in" | |||
case tapToPay = "built_in" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is deliberately left, to keep the analytics comparable over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I kept all the analytics strings deliberately unchanged.
case cardReaderSelectTypeTapToPayTapped = "card_present_select_reader_type_built_in_tapped" | ||
case cardReaderSelectTypeBluetoothTapped = "card_present_select_reader_type_bluetooth_tapped" | ||
case cardReaderDiscoveryFailed = "card_reader_discovery_failed" | ||
case cardReaderConnectionFailed = "card_reader_connection_failed" | ||
case cardReaderConnectionSuccess = "card_reader_connection_success" | ||
case cardReaderDisconnectTapped = "card_reader_disconnect_tapped" | ||
case manageCardReadersBuiltInReaderAutoDisconnect = "manage_card_readers_automatic_disconnect_built_in_reader" | ||
case manageCardReadersTapToPayReaderAutoDisconnect = "manage_card_readers_automatic_disconnect_built_in_reader" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing these strings are deliberately left, to keep the analytics comparable over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I kept all the analytics strings deliberately unchanged.
Depends on Stripe Terminal SDK update 4.2.0
Assigning everyone for a review for awareness. One is enough.
Description
A recent StripeSDK update renamed all the appleBuiltIn / localMobile references to TapToPay to match the branding.
I think it makes sense to do the same for Woo iOS codebase. It also helps with consistency, since we have all three used throughout the codebase: builtIn, local, tapToPay.
I didn't rename events to keep the analytics data consistent.
Steps to reproduce
Testing information
Tested on iPhone 14 Pro 17.7
Screenshots
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: