feat: Add Hook to monitor network connectivity status #56609
Merged
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
This code is non-functioning currently.
This bridge will be required for the planned JavaScript Hook to monitor connection status.
Provides React Hook for monitoring the network connection status via the bridge to the host app.
dcalhoun
added
Mobile App - i.e. Android or iOS
This reverts commit a8d3660.
|
Size Change: 0 B Total Size: 1.72 MB ℹ️ View Unchanged
|
This was referenced Nov 28, 2023
Merged
twstokes
reviewed
Nov 28, 2023
packages/react-native-editor/ios/GutenbergDemo/GutenbergViewController.swift
Outdated
Show resolved
Hide resolved
Semicolon is unnecessary. Co-authored-by: Tanner Stokes <tanner.stokes@automattic.com>
This bridge enables monitoring the connection status on Android.
Merged
12 tasks
Allow the Android platform to request the current network connection status.
The Demo editor fails to build without a mocked bridge method.
dcalhoun
force-pushed
the
feat/connectivity-status
branch
from
c075674
to
98b6977
Compare
dcalhoun
commented
Nov 29, 2023
| @@ -185,6 +190,49 @@ export function subscribeOnRedoPressed( callback ) { | |||
| return gutenbergBridgeEvents.addListener( 'onRedoPressed', callback ); | |||
| } | |||
|
|
|||
| export function useIsConnected() { | |||
There was a problem hiding this comment.
I'd like to move this useIsConnected and its related logic into a separate file to improve code comprehension and make testing easier. However, doing so would currently create a cyclic dependency.
After merging this work, I hope to refactor the bridge to avoid cyclic dependencies and enable testing.
There was a problem hiding this comment.
I feel we may want to consider adding more detail to this hook's name. In Jetpack, the concept of isConnected refers to a Jetpack site connection. Although it's a separate repo, being more descriptive in the hook name may help avoid confusion, and help also clarify "what" is connected. I had been using isNetworkConnected in a previous example, and even considered isMobileNetworkConnected to be extra clear. Open to other perspectives, however. If we do decide to update the name, it can happen in a follow-up PR.
There was a problem hiding this comment.
Good feedback. I agree we should update the name to more clearly communicate its intent: isNetworkConnected. I suggest we refactor this in a future PR alongside implementing tests.
This was referenced Nov 30, 2023
derekblank
approved these changes
Dec 1, 2023
| @@ -185,6 +190,49 @@ export function subscribeOnRedoPressed( callback ) { | |||
| return gutenbergBridgeEvents.addListener( 'onRedoPressed', callback ); | |||
| } | |||
|
|
|||
| export function useIsConnected() { | |||
There was a problem hiding this comment.
I feel we may want to consider adding more detail to this hook's name. In Jetpack, the concept of isConnected refers to a Jetpack site connection. Although it's a separate repo, being more descriptive in the hook name may help avoid confusion, and help also clarify "what" is connected. I had been using isNetworkConnected in a previous example, and even considered isMobileNetworkConnected to be extra clear. Open to other perspectives, however. If we do decide to update the name, it can happen in a follow-up PR.
dcalhoun
added a commit
that referenced
this pull request
Dec 6, 2023
|
I reverted this work in #56836 to avoid merging bridge changes into the WordPress host apps, as we do not plan to integrate a tagged release before the next WordPress host app release. Introducing bridge changes without a formal tagged release would've disrupted the release process. I'll open another PR to merge this work after the next WordPress host app release (23.9 on Dec 11). |
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.
Related
What?
Add a React Hook for monitoring the network connection status via the bridge to
the host app.
Why?
The network connection status will enable future features improving the UX when
a user's network connection changes, e.g. displaying status indicators.
Fixes wordpress-mobile/gutenberg-mobile#6409.
How?
Add a
useIsConnectedReact Hook that invokes bridge methods retrieving andmonitoring the network connection status from the host app. Relying upon the
host app will result in a single source of truth and mitigate conflicting
statuses.
Testing Instructions
Checkout this branch within the
gutenberg-mobilesubmodule.Start the Metro server via
npm run start:reset.Apply the following patch:
Test Diff
Configure the app to load the local Metro server:
LOCAL_GUTENBERG=true bundle exec pod install.localGutenbergMobilePathwithinlocal-builds.gradle.Build and run the WordPress app.
Launch the editor and verify the connection status displayed is accurate.
Toggle airplane mode, verify the connection status displayed is accurate.
Testing Instructions for Keyboard
n/a, this internal tool introduces no user-facing changes.
Screenshots or screencast
n/a, this internal tool introduces no visible changes.