-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Compose workspace #1452
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
Compose workspace #1452
Conversation
Config UI has now been recreated using Compose.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this, @LokiFrostGiant!
I've added small comments before I review the Kotlin code.
admob/app/src/main/java/com/google/samples/quickstart/admobexample/EntryChoiceActivity.kt
Outdated
Show resolved
Hide resolved
...b/app/src/main/java/com/google/samples/quickstart/admobexample/kotlin/MainComposeActivity.kt
Outdated
Show resolved
Hide resolved
...b/app/src/main/java/com/google/samples/quickstart/admobexample/kotlin/MainComposeActivity.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LokiFrostGiant our CI seems to be mad about some indentation errors, can you please take a look?
I have also added some comments bellow.
...b/app/src/main/java/com/google/samples/quickstart/admobexample/kotlin/MainComposeActivity.kt
Outdated
Show resolved
Hide resolved
Hi @thatfiredev , I made the changes, but I expect the indentation errors will happen again. Once the CI runs again and reproduces the errors for the new version of the app, please let me know and I will revisit it again. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LokiFrostGiant !
For review by Rosário Fernandes @thatfiredev .
Created an option to run Admob using Jetpack Compose. Separated functionalities for interstitial ads to increase flexibility, such as setting callback, initializing, and displaying. Demonstrates each functionality more clearly/isolated. Interstitial is now re-callable by this.
In the MainComposeActivity file that was created, please review Composables to advise on whether to continue passing Lambdas or if passing a Context would be better suitable in this scenario. Additionally, please review the Theme.kt and Color.kt files to suggest any changes that properly meet your expected standards/format.
Please note that any changes to Config have been undone for a separate PR by @millandavid .