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

[Android] drop support for Android 4.4 #2367

Merged
merged 2 commits into from Oct 19, 2018

Conversation

@bbarthec
Copy link
Member

commented Oct 5, 2018

What

Drops support for Android 4.4 (KitKat)
Updated targetSdkVersion = 28 (Android Pie)
Updated Gradle = 4.10.2
Added local CreditCardEntry.aar lib with version 1.4.9 as this version on github is not pushed to any maven/gradle/android repo.

Test Plan

NCL up & running

@bbarthec bbarthec requested review from sjchmiela, tsapeta and esamelson Oct 5, 2018

@bbarthec bbarthec force-pushed the @bbarthec/drop-android-4.4 branch 3 times, most recently from 56cfe23 to 4d6e0c9 Oct 5, 2018

@esamelson
Copy link
Member

left a comment

Thanks! This generally looks good to me, just a few questions:

  • was all of the .gradle reformatting intentional? there are so many changes in this diff it's pretty hard to tell what the actual changes are.
  • last time we updated the targetSdkVersion (to 26) we had to add support for notification channels and adaptive icons. Are there any changes like that for targeting SDK 28 that you're aware of, and did you try tapping through all of the screens in NCL to see if anything broke?

Let's hold off on merging this for now until after the release, since we're not upgrading the JSC version this release and so don't need to drop support for 4.4 yet.

@bbarthec

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

@esamelson regarding .gradle files: yes, it was intentional as AndroidStudio suggests reformatting over & over, so most changes swaps 4 spaces with 2 spaces indents.
Regarding upgrading targetSdkVersion I've briefly checked some random screens, but I'll check them all then.

@bbarthec bbarthec force-pushed the @bbarthec/drop-android-4.4 branch from 4d6e0c9 to 85c1136 Oct 16, 2018

@ide

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Does the app compile and run as expected with targetSdkVersion 26. If so, we'd prefer to keep unrelated changes in separate PRs -- one PR to drop API 19/20 and a separate PR to increase the targetSdkVersion.

@ide

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@esamelson looked at the API changes between 26 and 28 and it seems like there are very few breaking changes so I'll let him decide whether we want to separate this PR or take it as-is. In the future, separating unrelated changes is helpful so that it's easier to review and revert PRs and compartmentalize risk.

@bbarthec

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Moved upgrading part into #2446

@bbarthec bbarthec changed the title @bbarthec/drop android 4.4 [Android] drop support for Android 4.4 Oct 18, 2018

@bbarthec bbarthec added in review and removed in progress labels Oct 18, 2018

@esamelson
Copy link
Member

left a comment

looks great. Thank you!!

Let's wait on merging this until #2330 lands so that we can be sure to get working shell app bases for SDK 25-30 with Android 4.4 support.

@ide

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Looks like the Android CI failed due to network flakiness, and when I re-ran it the build passed: https://circleci.com/gh/expo/expo/2323 (build is canceled since it's an SSH build)

@esamelson esamelson merged commit b6df845 into master Oct 19, 2018

1 of 2 checks passed

client Workflow: client
Details
sdk Workflow: sdk
Details

@esamelson esamelson removed the in review label Oct 19, 2018

@tsapeta tsapeta deleted the @bbarthec/drop-android-4.4 branch Oct 24, 2018

ronlaureles pushed a commit to ronlaureles/expo that referenced this pull request May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.