[Gutenberg] Add isNetworkConnected capability #19680
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derekblank
added
the
Do Not Merge
This was referenced Nov 28, 2023
Generated by 🚫 Danger |
|
Found 1 violations: The PR caused the following dependency changes: +--- project :libs:editor
-| \--- org.wordpress-mobile.gutenberg-mobile:react-native-gutenberg-bridge:v1.109.0
-| +--- com.facebook.fresco:animated-gif:2.0.0
-| | +--- com.parse.bolts:bolts-tasks:1.4.0
-| | +--- com.facebook.soloader:soloader:0.6.0 -> 0.10.4 (*)
-| | +--- com.facebook.fresco:fbcore:2.0.0 -> 2.5.0
-| | \--- com.facebook.fresco:animated-base:2.0.0
-| | +--- com.facebook.fresco:fbcore:2.0.0 -> 2.5.0
-| | +--- com.facebook.fresco:imagepipeline-base:2.0.0 -> 2.5.0 (*)
-| | +--- com.facebook.fresco:imagepipeline:2.0.0 -> 2.5.0 (*)
-| | +--- com.facebook.fresco:animated-drawable:2.0.0
-| | | +--- com.facebook.fresco:imagepipeline:2.0.0 -> 2.5.0 (*)
-| | | +--- com.facebook.fresco:drawee:2.0.0 -> 2.5.0 (*)
-| | | \--- com.facebook.fresco:fbcore:2.0.0 -> 2.5.0
-| | \--- com.parse.bolts:bolts-tasks:1.4.0
-| +--- com.google.android.material:material:1.2.1 -> 1.9.0 (*)
-| +--- com.github.wordpress-mobile:react-native-video:5.2.0-wp-6
-| | +--- com.google.android.exoplayer:exoplayer:2.13.3
-| | | +--- com.google.android.exoplayer:exoplayer-core:2.13.3
-| | | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | | +--- com.google.android.exoplayer:exoplayer-common:2.13.3
-| | | | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | | | \--- com.google.guava:guava:27.1-android
-| | | | | +--- com.google.guava:failureaccess:1.0.1
-| | | | | \--- com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
-| | | | \--- com.google.android.exoplayer:exoplayer-extractor:2.13.3
-| | | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | | \--- com.google.android.exoplayer:exoplayer-common:2.13.3 (*)
-| | | +--- com.google.android.exoplayer:exoplayer-dash:2.13.3
-| | | | +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
-| | | | \--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | +--- com.google.android.exoplayer:exoplayer-hls:2.13.3
-| | | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | | \--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
-| | | +--- com.google.android.exoplayer:exoplayer-smoothstreaming:2.13.3
-| | | | +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
-| | | | \--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | +--- com.google.android.exoplayer:exoplayer-transformer:2.13.3
-| | | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | | \--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
-| | | \--- com.google.android.exoplayer:exoplayer-ui:2.13.3
-| | | +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
-| | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | +--- androidx.recyclerview:recyclerview:1.1.0 -> 1.3.0 (*)
-| | | \--- androidx.media:media:1.2.1 (*)
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | +--- androidx.core:core:1.1.0 -> 1.10.0 (*)
-| | +--- androidx.media:media:1.1.0 -> 1.2.1 (*)
-| | +--- com.google.android.exoplayer:extension-okhttp:2.13.3
-| | | +--- com.google.android.exoplayer:exoplayer-common:2.13.3 (*)
-| | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | | \--- com.squareup.okhttp3:okhttp:3.12.11 -> 4.11.0 (*)
-| | \--- com.squareup.okhttp3:okhttp:${OKHTTP_VERSION} -> 4.11.0 (*)
-| +--- com.github.wordpress-mobile:react-native-slider:3.0.2-wp-4
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-get-random-values:1.4.0
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-safe-area-context:4.6.3
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.20 -> 1.9.10 (*)
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-screens:3.22.0
-| | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.21 -> 1.9.10 (*)
-| | +--- androidx.appcompat:appcompat:1.4.1 -> 1.6.1 (*)
-| | +--- androidx.fragment:fragment:1.3.6 -> 1.5.7 (*)
-| | +--- androidx.coordinatorlayout:coordinatorlayout:1.1.0 (*)
-| | +--- androidx.swiperefreshlayout:swiperefreshlayout:1.0.0 -> 1.1.0 (*)
-| | +--- com.google.android.material:material:1.1.0 -> 1.9.0 (*)
-| | \--- androidx.core:core-ktx:1.5.0 -> 1.10.0 (*)
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-svg:13.9.0
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-webview:11.26.1
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.9.10 (*)
-| | \--- androidx.webkit:webkit:1.4.0 -> 1.7.0 (*)
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-masked-view:0.2.9
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-clipboard:1.11.2
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-fast-image:8.5.11
-| | +--- com.github.bumptech.glide:glide:4.12.0 -> 4.15.1
-| | | +--- com.github.bumptech.glide:gifdecoder:4.15.1
-| | | | \--- androidx.annotation:annotation:1.3.0 -> 1.6.0 (*)
-| | | +--- com.github.bumptech.glide:disklrucache:4.15.1
-| | | +--- com.github.bumptech.glide:annotations:4.15.1
-| | | +--- androidx.fragment:fragment:1.3.6 -> 1.5.7 (*)
-| | | +--- androidx.vectordrawable:vectordrawable-animated:1.1.0 (*)
-| | | +--- androidx.exifinterface:exifinterface:1.3.3 -> 1.3.6 (*)
-| | | \--- androidx.tracing:tracing:1.0.0 -> 1.1.0 (*)
-| | \--- com.github.bumptech.glide:okhttp3-integration:4.12.0
-| | +--- com.github.bumptech.glide:glide:4.12.0 -> 4.15.1 (*)
-| | +--- com.squareup.okhttp3:okhttp:3.9.1 -> 4.11.0 (*)
-| | \--- androidx.annotation:annotation:1.0.0 -> 1.6.0 (*)
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-reanimated:2.17.0
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-gesture-handler:2.10.2
-| | +--- org.wordpress-mobile.react-native-libraries.v3:react-native-reanimated:2.17.0
-| | +--- androidx.appcompat:appcompat:1.4.1 -> 1.6.1 (*)
-| | +--- androidx.core:core-ktx:1.6.0 -> 1.10.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.21 -> 1.9.10 (*)
-| +--- org.wordpress-mobile.react-native-libraries.v3:react-native-linear-gradient:2.7.3
-| +--- org.wordpress-mobile.gutenberg-mobile:react-native-aztec:v1.109.0
-| | +--- androidx.legacy:legacy-support-v4:1.0.0 (*)
-| | +--- androidx.gridlayout:gridlayout:1.0.0
-| | | +--- androidx.core:core:1.0.0 -> 1.10.0 (*)
-| | | \--- androidx.legacy:legacy-support-core-ui:1.0.0 (*)
-| | +--- androidx.cardview:cardview:1.0.0 (*)
-| | +--- androidx.appcompat:appcompat:1.2.0 -> 1.6.1 (*)
-| | +--- androidx.recyclerview:recyclerview:1.1.0 -> 1.3.0 (*)
-| | +--- org.wordpress:aztec:v1.8.0 (*)
-| | +--- org.wordpress.aztec:wordpress-shortcodes:v1.8.0 (*)
-| | +--- org.wordpress.aztec:wordpress-comments:v1.8.0 (*)
-| | +--- org.wordpress.aztec:glide-loader:v1.8.0
-| | | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.9.10 (*)
-| | | +--- org.wordpress:aztec:v1.8.0 (*)
-| | | \--- com.github.bumptech.glide:glide:4.10.0 -> 4.15.1 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.9.10 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.9.10 (*)
+| \--- org.wordpress-mobile.gutenberg-mobile:react-native-gutenberg-bridge:6415-f9dfaf7c64b481905f67ced3a754101ce79895c4 FAILED
+--- org.wordpress:login:1.10.0
-| \--- androidx.gridlayout:gridlayout:1.0.0 (*)
+| \--- androidx.gridlayout:gridlayout:1.0.0
+| +--- androidx.core:core:1.0.0 -> 1.10.0 (*)
+| \--- androidx.legacy:legacy-support-core-ui:1.0.0 (*)
+--- com.automattic:stories:2.4.0
-| \--- com.github.bumptech.glide:glide:4.10.0 -> 4.15.1 (*)
+| \--- com.github.bumptech.glide:glide:4.10.0 -> 4.15.1
+| +--- com.github.bumptech.glide:gifdecoder:4.15.1
+| | \--- androidx.annotation:annotation:1.3.0 -> 1.6.0 (*)
+| +--- com.github.bumptech.glide:disklrucache:4.15.1
+| +--- com.github.bumptech.glide:annotations:4.15.1
+| +--- androidx.fragment:fragment:1.3.6 -> 1.5.7 (*)
+| +--- androidx.vectordrawable:vectordrawable-animated:1.1.0 (*)
+| +--- androidx.exifinterface:exifinterface:1.3.3 -> 1.3.6 (*)
+| \--- androidx.tracing:tracing:1.0.0 -> 1.1.0 (*)
-\--- com.google.android.exoplayer:exoplayer:2.13.3 (*)
+\--- com.google.android.exoplayer:exoplayer:2.13.3
+ +--- com.google.android.exoplayer:exoplayer-core:2.13.3
+ | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ | +--- com.google.android.exoplayer:exoplayer-common:2.13.3
+ | | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ | | \--- com.google.guava:guava:27.1-android
+ | | +--- com.google.guava:failureaccess:1.0.1
+ | | \--- com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+ | \--- com.google.android.exoplayer:exoplayer-extractor:2.13.3
+ | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ | \--- com.google.android.exoplayer:exoplayer-common:2.13.3 (*)
+ +--- com.google.android.exoplayer:exoplayer-dash:2.13.3
+ | +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
+ | \--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ +--- com.google.android.exoplayer:exoplayer-hls:2.13.3
+ | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ | \--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
+ +--- com.google.android.exoplayer:exoplayer-smoothstreaming:2.13.3
+ | +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
+ | \--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ +--- com.google.android.exoplayer:exoplayer-transformer:2.13.3
+ | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ | \--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
+ \--- com.google.android.exoplayer:exoplayer-ui:2.13.3
+ +--- com.google.android.exoplayer:exoplayer-core:2.13.3 (*)
+ +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+ +--- androidx.recyclerview:recyclerview:1.1.0 -> 1.3.0 (*)
+ \--- androidx.media:media:1.2.1 (*)
Please review and act accordingly
|
dcalhoun
reviewed
Nov 28, 2023
| @@ -2504,7 +2505,8 @@ private GutenbergPropsBuilder getGutenbergPropsBuilder() { | |||
| postType, | |||
| hostAppNamespace, | |||
| featuredImageId, | |||
| themeBundle | |||
| themeBundle, | |||
| isNetworkConnected | |||
There was a problem hiding this comment.
My understanding of the props passed to the editor is that they are "initial props," i.e. they are never updated. Even if we did update them in the host app, I do not believe the editor would recognize updates; also, doing so might also be inefficient (re-rendering the entire editor). Am I mistaken?
Because of this, the approach I've drafted thus far for iOS in WordPress/gutenberg#56609 relies upon a bridge method and subscription to a bridge event.
How do you think we should reconcile the two, currently divergent approaches?
If you'd prefer me implement the Android side mimicking my current iOS approach, happy to do so — just let me know. Splitting the work by platform may be more difficult than I originally envisioned.
There was a problem hiding this comment.
From reviewing similar logic in the project, I came across featuredImageId implemented in WordPress/gutenberg#30806 and WordPress/gutenberg#31347. It appears to combine the approach of an initial prop with a subscription to receives updates to the value.
The pro of an initial prop would be avoiding the need for an asynchronous bridge request for the initial value for each usage of the Hook.
The cons might be:
- A need to pass the initial value down to Hook usages, i.e. using the global store or React Context (
featuredImageIdis stored on the entity in the global store); - And the initial prop would be an initial "seed" value and ignored after first render, which could be (possibly necessary) indirection.
"Taking a step back," we could place the value in the global store and hoist the subscription to the top of the editor like many other bridge subscriptions, but my initial thought is that this network connection state doesn't need to be in the global store or attached to the editor package. WDYT?
There was a problem hiding this comment.
How do you think we should reconcile the two, currently divergent approaches?
I agree that this approach was experiment, and I wasn't certain if it would actually work without implementing the connection logic subscription.
I think it would be most efficient for you to apply the same approach as in your iOS PRs. The ConnectionChangeReceiver events observe when connectivity has changed.
|
Closing in favor of WordPress/gutenberg#56609 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP to test sending network connectivity status to the Editor:
To Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.UI Changes Testing Checklist: