feat: Use androidx namespace
#44
Conversation
.github/workflows/readme.yml
Outdated
| - name: Android Lint | ||
| run: ./gradlew lint |
There was a problem hiding this comment.
Lint errors has been fixed so we can add this check
There was a problem hiding this comment.
Question (❓): Since for the Build library/example app steps you chose a debug build type, why did you chose both debug and release build type for the Android Lint step? Was that done by design? Maybe what you wanted was ./gradlew lintDebug instead?
There was a problem hiding this comment.
Good point, there's no reason to lint all build types!
| implementation 'com.android.support:appcompat-v7:28.0.0' | ||
| implementation 'com.android.support:customtabs:28.0.0' | ||
| implementation 'com.android.support:support-v4:28.0.0' | ||
| implementation 'com.android.support:support-media-compat:28.0.0' | ||
| implementation 'com.android.support:design:28.0.0' | ||
| implementation 'org.codehaus.jackson:jackson-mapper-lgpl:1.9.13' | ||
| implementation 'org.codehaus.jackson:jackson-core-lgpl:1.9.13' | ||
| implementation 'com.google.android.gms:play-services-ads:17.1.3' |
There was a problem hiding this comment.
Unused dependencies
| @Override | ||
| public boolean onCreateOptionsMenu(Menu menu) { | ||
| getMenuInflater().inflate(R.menu.main, menu); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The menu was not used
wzieba
force-pushed
the
publishing_scripts
branch
from
cbb58f2
to
2672680
Compare
There was a problem hiding this comment.
👋 @wzieba !
I reviewed and testing this PR. In doing so, I left a couple of questions (❓) for you to reply to. Let me know what you think and we can then progress with this work.
PS: This PR is about the usage and update to AndroidX. On that aspect everything looks okay. But, since you also made few other changes, Lint on CI and targetSdkVersion related, I took this opportunity to comment on those too. I hope my comments will help you! 🙏
.github/workflows/readme.yml
Outdated
| - name: Android Lint | ||
| run: ./gradlew lint |
There was a problem hiding this comment.
Question (❓): Since for the Build library/example app steps you chose a debug build type, why did you chose both debug and release build type for the Android Lint step? Was that done by design? Maybe what you wanted was ./gradlew lintDebug instead?
|
|
||
| defaultConfig { | ||
| applicationId "com.parsely.example.parselyexample" | ||
| minSdkVersion 21 | ||
| targetSdkVersion 28 |
There was a problem hiding this comment.
Question (❓): Why did you chose to completely remove the targetSdkVersion from both, the parsely library and the example app module?
I built and then run the example app. Then, on launching the HiParsely app, I got a permission screen first, and following that, a version dialog afterwards. I think this is done because of the fact that when targetSdkVersion is not set, the default value equals that given to minSdkVersion (see docs).
| Permission Screen | Version Dialog |
|---|---|
 |
 |
I then tried to point both, the parsely and example modules, both, their compileSdkVersion and targetSdkVersion values, to 32. Everything started working as expected. I also built the release build type (./gradlew assembleRelease) and it worked this time. Mind the fact, that this doesn't work as at its current state. Then, I triggered Lint for the release build type and it too worked okay (./gradlew lintRelease). 🎉
I wonder now... can't we use targetSdkVersion 32, is something blocking us to do so? 🤔
There was a problem hiding this comment.
Thanks, good point! Yes, let's specify targetSdkVersion to 32. I went too far with removing configuration code.
There was a problem hiding this comment.
Additionally, due to the permission issue you've pointed out, I've reverted my previous change of minSdk from 21 to 23 (as it was before).
Closes #43