Skip to content

Conversation

@toupper
Copy link
Contributor

@toupper toupper commented May 25, 2023

Closes: #9748

Description

With this PR we add the Woo logo to the Scan to Pay QR code, and also refactor the code to extract the logic to its own extensions. That way it both the main logic and its helper parts can be reused in other parts of the code.

Testing instructions

  1. Run the app
  2. Go to menu
  3. Go to Payments
  4. Tap on Collect Payment and follow the flow
  5. On the Payment Methods screen tap on Scan to Pay
  6. Notice the QR code has the Woo logo in the center. Scan the QR Code to make sure that it still links to the right URL.

Screenshots


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

@toupper toupper added type: task An internally driven task. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels May 25, 2023
@toupper toupper added this to the 13.8 milestone May 25, 2023
@toupper toupper requested review from iamgabrielma and joshheald May 25, 2023 15:24
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 25, 2023

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr9823-2d13b6d on your iPhone

If you need access to App Center, please ask a maintainer to add you.

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.

Tests well! I left a suggestion to refactor UIImage+Background for readability.

As a side note, I found an "issue" when scanning the barcode: As the initial site I used is a private site, I could scan it but it redirected me to the "coming soon" page rather than the payment page.

Perhaps we should hide the button if the site is private? Or add some sort of notice. We can check this by reaching the SessionManager's defaultSite.isPublic property.

🚢

Comment on lines 6 to 23
func withBackground(color: UIColor, opaque: Bool = true) -> UIImage {
UIGraphicsBeginImageContextWithOptions(size, opaque, scale)

guard let ctx = UIGraphicsGetCurrentContext(),
let image = cgImage else {
return self
}

defer { UIGraphicsEndImageContext() }

let rect = CGRect(origin: .zero, size: size)
ctx.setFillColor(color.cgColor)
ctx.fill(rect)
ctx.concatenate(CGAffineTransform(a: 1, b: 0, c: 0, d: -1, tx: 0, ty: size.height))
ctx.draw(image, in: rect)

return UIGraphicsGetImageFromCurrentImageContext() ?? self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot happening here, so I think we could improve it by:

  • Renaming ctx to currentGraphicsContext
  • Adding internal comments
  • Changing:
-  ctx.concatenate(CGAffineTransform(a: 1, b: 0, c: 0, d: -1, tx: 0, ty: size.height))
+ // Flips the context vertically, then translates it vertically
+ // By default, the coordinate system in Core Graphics is different from that of UIKit
+ currentGraphicsContext.scaleBy(x: 1, y: -1)
+ currentGraphicsContext.translateBy(x: 0, y: -size.height)
Suggested change
func withBackground(color: UIColor, opaque: Bool = true) -> UIImage {
UIGraphicsBeginImageContextWithOptions(size, opaque, scale)
guard let ctx = UIGraphicsGetCurrentContext(),
let image = cgImage else {
return self
}
defer { UIGraphicsEndImageContext() }
let rect = CGRect(origin: .zero, size: size)
ctx.setFillColor(color.cgColor)
ctx.fill(rect)
ctx.concatenate(CGAffineTransform(a: 1, b: 0, c: 0, d: -1, tx: 0, ty: size.height))
ctx.draw(image, in: rect)
return UIGraphicsGetImageFromCurrentImageContext() ?? self
}
func withBackground(color: UIColor, opaque: Bool = true) -> UIImage {
UIGraphicsBeginImageContextWithOptions(size, opaque, scale)
// Checks if there is a current graphics context, and the UIImage has a CGImage,
// return the original image otherwise
guard let currentGraphicsContext = UIGraphicsGetCurrentContext(),
let image = cgImage else {
return self
}
// Ensure removal of custom currentGraphicsContext on deallocation
defer { UIGraphicsEndImageContext() }
let rect = CGRect(origin: .zero, size: size)
currentGraphicsContext.setFillColor(color.cgColor)
currentGraphicsContext.fill(rect)
// Flips the context vertically, then translates it vertically
// By default, the coordinate system in Core Graphics is different from that of UIKit
currentGraphicsContext.scaleBy(x: 1, y: -1)
currentGraphicsContext.translateBy(x: 0, y: -size.height)
currentGraphicsContext.draw(image, in: rect)
// Return the UIImage from the current context,
// of the original image if happens to fail.
return UIGraphicsGetImageFromCurrentImageContext() ?? self
}

What do you think? I tested the changes to CGAfflineTransform and we get the same result , unless there's any other concern I've missed it seems that, from Apple docs, we shouldn't access this property directly.

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 for the comment! I added some of the suggestions here in 0d17ba5 Some parts of the code were self-explanatory and didn't need comments, but some others were indeed abstruse

@toupper
Copy link
Contributor Author

toupper commented May 26, 2023

As a side note, I found an "issue" when scanning the barcode: As the initial site I used is a private site, I could scan it but it redirected me to the "coming soon" page rather than the payment page.

Perhaps we should hide the button if the site is private? Or add some sort of notice. We can check this by reaching the SessionManager's defaultSite.isPublic property.

Thanks for the comments @iamgabrielma ! I will start a discussion about this 👍

@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@toupper toupper enabled auto-merge May 26, 2023 07:48
@toupper toupper merged commit 873234b into trunk May 26, 2023
@toupper toupper deleted the issue/9748-add-woo-logo-qr-code-scan-to-pay branch May 26, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Scan to Pay] Add Woo logo to the QR Code

4 participants