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.

Sign up for GitHub

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

[Gutenberg] Add isNetworkConnected capability #19680

Closed
wants to merge 2 commits into from
Closed

[Gutenberg] Add isNetworkConnected capability #19680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d162d9f
Add isNetworkConnected prop
derekblank Nov 28, 2023
db8fd45
Update Gutenberg ref
derekblank Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,6 +2475,7 @@ private GutenbergPropsBuilder getGutenbergPropsBuilder() {
boolean isFreeWPCom = mSite.isWPCom() && SiteUtils.onFreePlan(mSite);
boolean isWPComSite = mSite.isWPCom() || mSite.isWPComAtomic();
boolean shouldUseFastImage = !mSite.isPrivate() && !mSite.isPrivateWPComAtomic();
boolean isNetworkConnected = true;

String hostAppNamespace = mBuildConfigWrapper.isJetpackApp() ? "Jetpack" : "WordPress";

Expand Down Expand Up @@ -2504,7 +2505,8 @@ private GutenbergPropsBuilder getGutenbergPropsBuilder() {
postType,
hostAppNamespace,
featuredImageId,
themeBundle
themeBundle,
isNetworkConnected
Copy link
Member

@dcalhoun dcalhoun Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. A need to pass the initial value down to Hook usages, i.e. using the global store or React Context (featuredImageId is stored on the entity in the global store);
  2. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

);
}

Expand All @@ -2531,7 +2533,8 @@ private GutenbergPropsBuilder getGutenbergPropsBuilder() {
postType,
hostAppNamespace,
featuredImageId,
themeBundle
themeBundle,
isNetworkConnected
);
}

Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ ext {
automatticRestVersion = '1.0.8'
automatticStoriesVersion = '2.4.0'
automatticTracksVersion = '3.3.0'
gutenbergMobileVersion = 'v1.109.0'
gutenbergMobileVersion = '6415-f9dfaf7c64b481905f67ced3a754101ce79895c4'
wordPressAztecVersion = 'v1.8.0'
wordPressFluxCVersion = '2.57.0'
wordPressLoginVersion = '1.10.0'
Expand Down
6 changes: 4 additions & 2 deletions libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ data class GutenbergPropsBuilder(
private val postType: String,
private val hostAppNamespace: String,
private val featuredImageId: Int,
private val editorTheme: Bundle?
private val editorTheme: Bundle?,
private val isNetworkConnected: Boolean,
) : Parcelable {
fun build(activity: Activity, isHtmlModeEnabled: Boolean) = GutenbergProps(
enableContactInfoBlock = enableContactInfoBlock,
Expand Down Expand Up @@ -60,6 +61,7 @@ data class GutenbergPropsBuilder(
editorTheme = editorTheme,
translations = GutenbergUtils.getTranslations(activity),
isDarkMode = GutenbergUtils.isDarkMode(activity),
htmlModeEnabled = isHtmlModeEnabled
htmlModeEnabled = isHtmlModeEnabled,
isNetworkConnected = isNetworkConnected,
)
}
Loading