Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NTV-612: Move Minimum Version Up To iOS 14 #1751

Merged
merged 4 commits into from Oct 27, 2022
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Oct 25, 2022

πŸ“² What

Updates minimum iOS deployment target to 14.0.

πŸ€” Why

With the release of iOS 16, let's keep our usual support of two minimum versions back on iOS.

πŸ›  How

  1. Set the minimum deployment target in the project settings, address deprecation warnings.
  2. Nothing new that we implement was deprecated in iOS 16 specifically. Just a few things in iOS 14 (see code comments)

βœ… Acceptance criteria

  • App runs on iOS 14+
  • No regressions

Notes

In the last calendar year we've had:

  • Engaged sessions, "The number of sessions that lasted longer than 10 seconds, or had a conversion event, or had 2 or more screen or page views", by iOS 14.8+ users
  • Total number of active users at iOS version 14.6+

So, we shouldn't see any users being dropped from this update.

see our firebase console for more current data points.

@scottkicks scottkicks added the WIP label Oct 25, 2022
@scottkicks scottkicks added this to the release-5.6.0 milestone Oct 25, 2022
@scottkicks scottkicks self-assigned this Oct 25, 2022
@@ -79,7 +79,6 @@ extension User {
var followerNotification: Bool?
var friendActivityNotification: Bool?
var messagesNotification: Bool?
var postLikesNotification: Bool?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing unused property

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea leave this in, not iOS 14 related, when I created these models I left properties that might later get included in the GQL layer. It's good to know they are there as they indicate what v1 supported, and there are more of them.

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #1751 (763105a) into main (0d52877) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main    #1751   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files        1275     1275           
  Lines      115181   115183    +2     
  Branches    30454    30455    +1     
=======================================
+ Hits        98350    98352    +2     
+ Misses      15778    15777    -1     
- Partials     1053     1054    +1     
Impacted Files Coverage Ξ”
...anksProjects/Controller/ThanksViewController.swift 56.74% <33.33%> (-0.65%) ⬇️
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@scottkicks scottkicks marked this pull request as ready for review October 25, 2022 17:30
@msadoon msadoon added needs review and removed WIP labels Oct 26, 2022
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Overall looks good, just make a few changes as suggested.

@@ -124,7 +124,7 @@ private func shippingRulesData(

return ShippingRule.shippingRule(from: fragment)
}
.flatMap { $0 }
.compactMap { $0 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this in, there are other deprecated warnings of using .flatMap where we can use .compactMap in the model layer, but more investigation needs to be done as to why we're using compactMap and then again compactMap the same collection. Seems like we need flatMap to further reduce the number of levels in the collection.

@@ -79,7 +79,6 @@ extension User {
var followerNotification: Bool?
var friendActivityNotification: Bool?
var messagesNotification: Bool?
var postLikesNotification: Bool?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yea leave this in, not iOS 14 related, when I created these models I left properties that might later get included in the GQL layer. It's good to know they are there as they indicate what v1 supported, and there are more of them.

@msadoon
Copy link
Contributor

msadoon commented Oct 26, 2022

just wait until tests pass and clean up the formatting then merge

@scottkicks scottkicks merged commit d954d24 into main Oct 27, 2022
@scottkicks scottkicks deleted the ntv-612/minimum-version-14 branch October 27, 2022 13:30
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.

None yet

2 participants