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

Add SwiftUI and some compose refactors #201

Merged
merged 22 commits into from Aug 5, 2021

Conversation

russhwolf
Copy link
Contributor

@russhwolf russhwolf commented Jul 27, 2021

Issue: https://github.com/touchlab/KaMPKit/issues/[issue number]

[Platform]

  • iOS
  • Android
  • KMP

[Summary]

[Fix]

[Testing]

Reviewer Tips:
* Use "Nitpick:" if it's a minor non-crucial request.
* If you're done with comments either end with a review or comment something helpful like "done with comments for now"

brady-aiello and others added 15 commits March 31, 2021 09:10
[Description]
Preparing for introducing Compose, which requires Android Studio Canary,
which requires Gradle 7+

[Solution]
- Upgrade Gradle and Android Gradle Plugin
- Make necessary changes to accomodate the new Gradle and AGP APIs

[Testing]
- ./gradlew build
- manual testing of Android and iOS apps
[Description]
Jetpack Compose is going stable in July 2021, and we want to show folks
the easiest way to get started with KMM using Compose.

[Solution]
- Add Compose UI, and support SwipeRefresh using Accompanist.
- Remove XML layouts.

[Testing]
- ./gradlew :shared:build :app:build
- manual testing
[Summary]
The Loading UI is usually shown in conjunction with other UI, such as an
error view, or valid data. Consuming it with a when statement and
treating it like any other state may not make sense with Compose and
Swift UI. I tried a different approach here, giving the Loading state
the last valid DataState as well:
#6

[Fix]
In this PR, I'm using the classic NetworkBoundResource approach, where
the DataState is not a sealed class; instead, it contains nullable
versions of all possible states.

[Testing]
- `./gradlew :app:build`
- `./gradlew :shared:build`
- `xcodebuild -workspace ios/KaMPKitiOS.xcworkspace -scheme KaMPKitiOS
    -sdk iphoneos -configuration Debug build -destination name="iPhone 8"`
- manual testing
import org.koin.androidx.viewmodel.ext.android.viewModel
import org.koin.core.component.KoinComponent
import org.koin.core.component.inject
import org.koin.core.parameter.parametersOf

class MainActivity : AppCompatActivity(), KoinComponent {
class MainActivity : ComponentActivity(), KoinComponent {
Copy link
Contributor

Choose a reason for hiding this comment

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

AppCompatActivity is a ComponentActivity; why the change?

Copy link
Contributor

@kpgalligan kpgalligan left a comment

Choose a reason for hiding this comment

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

LGTM. I need to learn both of these things better, for sure.

Copy link
Contributor

@mrf7 mrf7 left a comment

Choose a reason for hiding this comment

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

Small change if you wanna do it

color = MaterialTheme.colors.background,
modifier = Modifier.fillMaxSize()
) {
val isRefreshingState by remember(dogsState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this can be changed to just isRefreshing = dogsState.loading since its not doing any calculation, or just removed and do rememberSwipeRefreshState(isRefreshing = dogsState.loading) below

@russhwolf russhwolf merged commit 4bf0000 into touchlab:main Aug 5, 2021
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

4 participants