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

fix(Android): Present the component immediately if there's only one #106

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

tido64
Copy link
Member

@tido64 tido64 commented Jun 9, 2020

android-startup

@tido64 tido64 added the platform: Android This affects Android label Jun 9, 2020
@tido64 tido64 self-assigned this Jun 9, 2020
@arazabishov
Copy link
Member

Hmm, you will lose access to the dev menu options on start. Also, backstack behaviour will be somewhat unintuitive where it gets you back to a screen that you haven't seen when launching the app.

It makes a lot of sense on desktop where you land to a component on start and always have access to contextual menus. If we want to make it "cleaner" on mobile I would suggest to make dev menu somehow accessible everywhere and do not allow to go back to the list of components.

@tido64
Copy link
Member Author

tido64 commented Jun 10, 2020

If we want to make it "cleaner" on mobile I would suggest to make dev menu somehow accessible everywhere and do not allow to go back to the list of components.

I tried replacing the Activity as soon as I am able to detect that we only have one component. The issue I hit next was that we'd always load the bundle from disk, so I turned it on as long as it is a debug build. I think the shaker menu is enough for most use cases. The context menu we provide is mostly for convenience. What do you think?

diff --git a/android/app/src/main/java/com/sample/MainActivity.kt b/android/app/src/main/java/com/sample/MainActivity.kt
index 52d2215..9ed7004 100644
--- a/android/app/src/main/java/com/sample/MainActivity.kt
+++ b/android/app/src/main/java/com/sample/MainActivity.kt
@@ -1,5 +1,6 @@
 package com.sample

+import android.content.Intent
 import android.os.Bundle
 import android.view.LayoutInflater
 import android.widget.TextView
@@ -56,12 +57,19 @@ class MainActivity : ReactActivity() {
         val manifest = manifestProvider.manifest
             ?: throw IllegalStateException("app.json is not provided or TestApp is misconfigured")

-        setupToolbar(manifest.displayName)
-        setupRecyclerView(manifest.components)
-
         if (manifest.components.count() == 1) {
-            startComponentActivity(createComponentViewModel(manifest.components[0]))
+            val component = createComponentViewModel(manifest.components[0])
+            startActivity(ComponentActivity.newIntent(
+                this, component.name, component.displayName, component.initialProperties
+            ).apply {
+                flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_TASK_ON_HOME
+            })
+            finish()
+            return
         }
+
+        setupToolbar(manifest.displayName)
+        setupRecyclerView(manifest.components)
     }

     private fun setupToolbar(displayName: String) {
diff --git a/android/app/src/main/java/com/sample/react/TestAppReactNativeHost.kt b/android/app/src/main/java/com/sample/react/TestAppReactNativeHost.kt
index 10d2523..df7728f 100644
--- a/android/app/src/main/java/com/sample/react/TestAppReactNativeHost.kt
+++ b/android/app/src/main/java/com/sample/react/TestAppReactNativeHost.kt
@@ -10,6 +10,7 @@ import com.facebook.react.bridge.ReactMarker
 import com.facebook.react.bridge.ReactMarkerConstants
 import com.facebook.react.common.LifecycleState
 import com.facebook.soloader.SoLoader
+import com.sample.BuildConfig
 import javax.inject.Inject
 import javax.inject.Singleton

@@ -39,12 +40,12 @@ class TestAppReactNativeHost @Inject constructor(
     application: Application,
     private val reactBundleNameProvider: ReactBundleNameProvider
 ) : ReactNativeHost(application) {
-    var source: BundleSource = reactBundleNameProvider.bundleName
-        ?.let { BundleSource.Disk } ?: BundleSource.Server
+    var source: BundleSource = BuildConfig.DEBUG
+        .let { BundleSource.Server } ?: BundleSource.Disk

     fun reload(activity: Activity, newSource: BundleSource) {
         assert(hasInstance()) {

@arazabishov
Copy link
Member

The issue I hit next was that we'd always load the bundle from disk, so I turned it on as long as it is a debug build.
Ah, I see. I would rather keep the initial implementation then.

Do you think it will make sense to implement a different navigation pattern where having a default component feels natural? Something like a right hand side sliding drawer? For example:

Obviously, this will require more work and thought. We can get the PR in as the first implementation, but I would like us to think of other solutions too. @sweggersen any input on this?

@sweggersen
Copy link
Member

sweggersen commented Jun 12, 2020

I personally think that BuildConfig.DEBUG is not the correct flag to spot intent, since basically all builds you try in AS will be DEBUG.

I've had bad experiences when the app tries to be smart and loads the bundle from disk if available. I have to click load from dev server every time.
Can we do something like this:

image

Remember choice can be overridden later in a simple settings activity.

@arazabishov
Copy link
Member

@sweggersen How would you access the settings screen when working with a single experience only?

@sweggersen
Copy link
Member

sweggersen commented Jun 12, 2020

Right. My diagram is a bit off on that part. I thought we would launch the single experience Activity automatically, but still launch the experience list first, so that when you exit the experience, you would go back to the experience list where we have control of the toolbar menu, and have it there.

@tido64
Copy link
Member Author

tido64 commented Jun 12, 2020

On iOS, RN checks if the dev server is running first, then load the embedded bundle as fallback. I think we should implement (and upstream?) that feature. As @sweggersen experienced, I think it's wrong that it tries the embedded bundle first. As our users are developers, it's more likely that they want the dev server most of the time. I've created the issue here: #115. Let's revisit this after it is resolved.

@tido64 tido64 force-pushed the tido/android-load-lone-component branch from 2972a0d to ef415eb Compare June 15, 2020 19:37
@tido64
Copy link
Member Author

tido64 commented Jun 15, 2020

Do you think it will make sense to implement a different navigation pattern where having a default component feels natural?

@arazabishov: Can we simply keep the current toolbar? We only have a few menu items, I'm not sure we need a whole right panel. In any case, it would be nice to get this in first and create a separate issue for it.

@tido64
Copy link
Member Author

tido64 commented Jun 15, 2020

Opened #117

@tido64 tido64 merged commit 0687e77 into master Jun 15, 2020
@tido64 tido64 deleted the tido/android-load-lone-component branch June 15, 2020 20:32
github-actions bot pushed a commit that referenced this pull request Jun 15, 2020
## [0.1.9](0.1.8...0.1.9) (2020-06-15)

### Bug Fixes

* **Android:** Present the component immediately if there's only one ([#106](#106)) ([0687e77](0687e77))
@tido64
Copy link
Member Author

tido64 commented Jun 15, 2020

🎉 This PR is included in version 0.1.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: Android This affects Android released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants