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

refactor: remove unused libraries #980

Merged
merged 32 commits into from
Nov 4, 2019

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Oct 24, 2019

The initial idea here was to remove the legacy support library, since it seems like it is not being used anywhere in the code. But then I noticed that if we use the Material Components Theme, we might be able to remove some other libraries as well, such as androidx.cardview:cardview:1.0.0.

I was wondering if this is something we're interested in? If it is, I can do it to the remaining samples on this same PR.

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

@rosariopfernandes just a few small questions but in general I am a fan of this!

functions/app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
@thatfiredev
Copy link
Member Author

thatfiredev commented Oct 25, 2019

Progress Tracking:

  • AdMob
  • Analytics
  • App Indexing
  • Authentication
  • Remote Config
  • Crashlytics
  • Realtime Database
  • Dynamic Links
  • Cloud Firestore
  • Cloud Functions
  • In-App Messaging
  • Cloud Messaging
  • ML Kit
  • ML Kit Language ID
  • ML Kit Smart Reply
  • ML Kit Translate Text
  • Performance Monitor
  • Cloud Storage

@@ -25,7 +25,7 @@
android:hint="@string/help_text" />
</com.google.android.material.textfield.TextInputLayout>

<androidx.constraintlayout.Guideline
<androidx.constraintlayout.widget.Guideline
Copy link
Member Author

Choose a reason for hiding this comment

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

The previous implementation was causing the app to crash on start. Seems like there was a breaking change on the latest version of ConstraintLayout. Found this solution on StackOverflow

@samtstern
Copy link
Contributor

@rosariopfernandes just let me know when you're ready for review! Doesn't have to be 100% done you can always do RTDB and MLKit in a follow-up

@thatfiredev
Copy link
Member Author

Thanks @samtstern , but I've already done both of them.
This is ready for review now.

@thatfiredev thatfiredev marked this pull request as ready for review November 4, 2019 20:23
@@ -34,7 +34,7 @@ class ChooserActivity : AppCompatActivity(), AdapterView.OnItemClickListener {
setContentView(R.layout.activity_chooser)

// Set up Adapter
val adapter = MyArrayAdapter(this, android.R.layout.simple_list_item_2, CLASSES)
val adapter = MyArrayAdapter(this, android.R.layout.simple_list_item_2, CLASSES as Array<Class<*>>)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated, but Android Studio was showing an error saying it expected Array<Class<*>>, but found Array<out Class<out Any>>.

public ImagePagerAdapter(FragmentManager fm, ImageInfo[] infos) {
super(fm);
super(fm, BEHAVIOR_RESUME_ONLY_CURRENT_FRAGMENT);
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the constant to match its Kotlin equivalent

@@ -218,6 +219,7 @@ class MainActivity : AppCompatActivity() {
* A [FragmentPagerAdapter] that returns a fragment corresponding to
* one of the sections/tabs/pages.
*/
@SuppressLint("WrongConstant")
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to ignore Lint message:

Must be one of: androidx.fragment.app.FragmentPagerAdapter.BEHAVIOR_RESUME_ONLY_CURRENT_FRAGMENT or androidx.fragment.app.FragmentPagerAdapter.BEHAVIOR_SET_USER_VISIBLE_HINT

@samtstern
Copy link
Contributor

@rosariopfernandes thank you SO much for doing all this! LGTM if it builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants