Skip to content

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Nov 16, 2022

Based on #8094 , that PR should be merged first.

Description

By updating the minimum release version to iOS 15 recently, we've seen some deprecation warnings. This PR fixes the case for 'imageEdgeInsets' was deprecated in iOS 15.0: This property is ignored when using UIButtonConfiguration

On some buttons, we rely on the distributeTitleAndImage(_:) extension in order to add the correct padding between image and text in a UIButton. However, since we're moving to use UIButton.Configuration now, this functionality comes for free by calling .imagePadding() instead.

Changes

  • Deleted UIButton+TitleAndImage
  • Added .imagePadding() usage to the 4 cases where we use this extension.

With the change, we can also change how we do other things, like setting title or image, but this is outside of the scope of this PR, which only intends to remove the Xcode deprecation warnings. We'll leave that for a different PR :)

Testing instructions

Confirm that the following buttons still keep the same separation between Image and Text:

  • Case 1: Go to Orders > open an Order > Check the "+ Add Customer Note" and "+ "Add Note" buttons.
  • Case 2: Go to Orders > open a "pending payment" Order > Tap on "Collect Payment" > Select "Card" > See the "Learn More about In-Person Payments"
  • Case 3: Go to Products > Tap on a Product > See the "+ Add more details" at the bottom of the view.
Case 1 Case 2 Case 3

@iamgabrielma iamgabrielma added type: technical debt Represents or solves tech debt of the project. status: draft This is a draft, still need more work but can be reviewed and commented if asked. iOS 15 Specific to iOS 15 and removed status: draft This is a draft, still need more work but can be reviewed and commented if asked. labels Nov 16, 2022
Comment on lines 284 to 286
// Only for testing, switch back to true before merging
//auxiliaryButton.isHidden = true
auxiliaryButton.isHidden = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for convenience when testing the Case 2, has to be removed before merging.

@iamgabrielma iamgabrielma added this to the 11.3 milestone Nov 16, 2022
@iamgabrielma iamgabrielma modified the milestones: 11.3, 11.4 Nov 18, 2022
Copy link
Contributor

@ealeksandrov ealeksandrov left a comment

Choose a reason for hiding this comment

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

Code looks good and described test cases work well!
But I've found one regression in BottomButtonContainerView:

There is a conflict between 2 UIButton.Configurations, so "primary button" loses its custom background and white text blends with white superview.

if viewModel.auxiliaryButtonimage != nil {
auxiliaryButton.distributeTitleAndImage(spacing: 8.0)
var config = UIButton.Configuration.plain()
config.imagePadding = CGFloat(Constants.buttonTitleAndImageSpacing)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: buttonTitleAndImageSpacing is already declared as CGFloat, no need to wrap/convert.
Applies also to 3 similar cases below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: buttonTitleAndImageSpacing is already declared as CGFloat, no need to wrap/convert. Applies also to 3 similar cases below.

Oops! Removed on 3d9760b

Base automatically changed from task/contentedgeinsets-deprecated-ios-15 to trunk November 24, 2022 01:07
@iamgabrielma iamgabrielma modified the milestones: 11.4, 11.5 Nov 24, 2022
@iamgabrielma
Copy link
Contributor Author

There is a conflict between 2 UIButton.Configurations, so "primary button" loses its custom background and white text blends with white superview.

Good catch! I didn't see it as I was testing it in Dark Mode 🙇

This has been changed on e67f70e by leaving the UIButton.Configuration responsibility to applyPrimaryButtonStyle(), then overriding only the cases when applyLinkButtonStyle() has an image. As this instance is only used by the ProductFormViewController, for now I preferred to leave it there rather than potentially modifying the button configuration to all buttons that apply the link style.

@wpmobilebot
Copy link
Collaborator

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 pr8129-ab8288b on your iPhone

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

Copy link
Contributor

@ealeksandrov ealeksandrov left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:
One note: can we merge it as a betafix to release/11.4, so we fix this minor alignment issue before reaching the users?

@iamgabrielma iamgabrielma changed the base branch from trunk to release/11.4 November 29, 2022 02:30
@iamgabrielma iamgabrielma merged commit a997e2c into release/11.4 Nov 29, 2022
@iamgabrielma iamgabrielma deleted the task/imageEdgeInsets-deprecated-ios15 branch November 29, 2022 02:31
@iamgabrielma iamgabrielma modified the milestones: 11.5, 11.4 ❄️ Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS 15 Specific to iOS 15 type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants