Skip to content

Conversation

@toupper
Copy link
Contributor

@toupper toupper commented Nov 16, 2022

Closes: #8119

Description

We remove here some wrapper functions around Apple's withTintColor because we didn't have any added value with them.

Testing instructions

Please check that the primary, secondary, and tertiary buttons still have the intended background and border colors. For instance, the 'Next' button in IssueRefundViewController displays correctly:


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

@toupper toupper added the type: technical debt Represents or solves tech debt of the project. label Nov 16, 2022
@toupper toupper added this to the 11.3 milestone Nov 16, 2022
@toupper toupper requested a review from iamgabrielma November 16, 2022 16:42
@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

@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 pr8139-c5b0e84 on your iPhone

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

@iamgabrielma iamgabrielma self-assigned this Nov 17, 2022
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.

LGTM. I left 2 questions in the comments 👍

return UIImage.gridicon(.camera)
.imageFlippedForRightToLeftLayoutDirection()
.applyTintColor(.placeholderImage)!
.withTintColor(.placeholderImage)
Copy link
Contributor

@iamgabrielma iamgabrielma Nov 17, 2022

Choose a reason for hiding this comment

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

💯

While checking why would we be force-unwrapping this, I saw there's several instances of imageWithTintColor(tintColor)! as well, which has been renamed to withTintColor(_:) and the force-unwrapping can be removed too. Would make sense to fix these as well as part of this PR? Or I can open a new issue otherwise.

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 hint here @iamgabrielma
We have some references to the WordPressUI function imageWithTintColor, which returns an optional, hence the force unwrapping. I didn't change those, because I didn't know if by doing so we would get any unexpected change (imageWithTintColor has its own implementation that might differ from Apple's). Because of that, I think creating a new issue to check on those is the right course of action here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! I opened a new one here: #8151

0366EAE12909A37800B51755 /* JustInTimeMessageAnnouncementCardViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0366EAE02909A37800B51755 /* JustInTimeMessageAnnouncementCardViewModel.swift */; };
036CA6F129229C9E00E4DF4F /* IndefiniteCircularProgressViewStyle.swift in Sources */ = {isa = PBXBuildFile; fileRef = 036CA6F029229C9E00E4DF4F /* IndefiniteCircularProgressViewStyle.swift */; };
036CA6B9291E8D4B00E4DF4F /* CardPresentModalPreparingReader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 036CA6B8291E8D4B00E4DF4F /* CardPresentModalPreparingReader.swift */; };
036CA6F129229C9E00E4DF4F /* IndefiniteCircularProgressViewStyle.swift in Sources */ = {isa = PBXBuildFile; fileRef = 036CA6F029229C9E00E4DF4F /* IndefiniteCircularProgressViewStyle.swift */; };
Copy link
Contributor

Choose a reason for hiding this comment

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

It would seem we've inadvertently moved this file in the project structure? I can't see any other changes 🤔

Copy link
Contributor Author

@toupper toupper Nov 17, 2022

Choose a reason for hiding this comment

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

That was automatically done by Xcode when deleting UIImage+TintColor.swift , but please note that the order of the files in that pbxproj field doesn't match the order in the project structure, so I wouldn't worry about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change applyTintColorToiOS13(_:) with withTintColor(_:)

4 participants