Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Dec 2, 2022

The app targets iOS 14.0+ making them always true.

Testing

If CI builds, we're good.

See also

Regression Notes

  1. Potential unintended areas of impact – N.A.
  2. What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
  3. What automated tests I added (or what prevented me from doing so) – N.A.

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes. N.A.
  • I have considered adding accessibility improvements for my changes. N.A.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

@mokagio mokagio requested a review from a team as a code owner December 2, 2022 05:40
Comment on lines 88 to +91
showUpdateSiteIconAlert()
}

if #available(iOS 14.0, *) {
showSiteIconSelectionAlert()
} else {
showUpdateSiteIconAlert()
}
showSiteIconSelectionAlert()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this code might possibly show two alerts? if FeatureFlag.siteIconCreator.enabled == false it'll call showUpdateSiteIconAlert() and showSiteIconSelectionAlert().

Is that right?

Or, should we add a return to the successful feature flag check branch?

cc @momo-ozawa who's the last one who touched this code. Thanks!

Copy link
Contributor

@momo-ozawa momo-ozawa Dec 6, 2022

Choose a reason for hiding this comment

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

Good catch - I totally missed this. Thanks @mokagio!

I've addressed this in a separate PR since this one already got merged to trunk. #19740

Copy link
Contributor Author

@mokagio mokagio Dec 6, 2022

Choose a reason for hiding this comment

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

That's great! 🙌

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 2, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19709-525027e on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 2, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19709-525027e on your iPhone

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

@mokagio mokagio self-assigned this Dec 2, 2022
@mokagio mokagio added this to the 21.4 milestone Dec 2, 2022
The app targets iOS 14.0+ making them always true.
return allowedType.conforms(to: requiredType)
} else {
return UTTypeConformsTo($0 as CFString, uttype)
guard let allowedType = UTType($0), let requiredType = UTType(uttype as String) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this let requiredType = UTType(uttype as String) part can be moved outside of the loop.

@mokagio mokagio merged commit 8f88d03 into trunk Dec 6, 2022
@mokagio mokagio deleted the mokagio/remove-available-ios-14 branch December 6, 2022 08:36
@momo-ozawa momo-ozawa mentioned this pull request Dec 6, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants