[RNMobile] Add OfflineStatus component to display network connectivity #56627
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
[Type] Enhancement
|
Size Change: +2.43 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
dcalhoun
reviewed
Nov 29, 2023
packages/edit-post/src/components/layout/offline-icon.native.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/layout/offline-status.native.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/layout/offline-status.native.js
Outdated
Show resolved
Hide resolved
derekblank
added
the
Mobile App - Automation
2 tasks
dcalhoun
reviewed
Dec 5, 2023
There was a problem hiding this comment.
The latest changes look good. I noted a few things while testing.
First, we likely need to implement dark mode support. Currently, the status text is illegible when dark mode is enabled.
iOS Screenshot
Second, after relocating OfflineStatus to the editor package, the testing instructions result in a cyclic dependency that throws an error. If we intend to use OfflineStatus in block-editor we may need to relocate OfflineStatus — possibly to the block-editor.
The cyclic dependency from the testing instructions is noted in this diagram with a red arrow.
Error Details
ERROR Error: Cannot unlock an undefined object., js engine: hermes
at Gutenberg (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:120500:38)
at RCTView
at View (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48203:43)
at RCTView
at View (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48203:43)
at AppContainer (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48081:36)
at gutenberg(RootComponent) (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:84470:28)
ERROR Error: Cannot unlock an undefined object., js engine: hermes
at Gutenberg (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:120500:38)
at RCTView
at View (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48203:43)
at RCTView
at View (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48203:43)
at AppContainer (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:48081:36)
at gutenberg(RootComponent) (http://localhost:8081/index.bundle//&platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.wordpress.gutenberg.development:84470:28)
ERROR TypeError: Cannot read property 'default' of undefined
This error is located at:
in Gutenberg
in RCTView (created by View)
in View (created by AppContainer)
in RCTView (created by View)
in View (created by AppContainer)
in AppContainer
in gutenberg(RootComponent), js engine: hermes
ERROR TypeError: Cannot read property 'default' of undefined
This error is located at:
in Gutenberg
in RCTView (created by View)
in View (created by AppContainer)
in RCTView (created by View)
in View (created by AppContainer)
in AppContainer
in gutenberg(RootComponent), js engine: hermes
|
Additional suggestion: For changes dependent another branch, I generally build atop that branch. That might make it easier to test the changes and ensure the integration is without issue. That said, I fully recognize this work was started prior to that branch existing. In this case I might build atop #56609. I would then open the PR against git switch rnmobile/offline-status
git fetch
git rebase --onto origin/trunk origin/feat/connectivity-status rnmobile/offline-statusNote the above requires that the I do not share to ask that it be done, merely to share my workflow. Merging the work in hidden pieces is a legitimate approach is well. |
|
Flaky tests detected in e97417d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7111347950
|
Updated to include dark mode support. 👍
I encountered this as well when testing, and agree that the block-editor package may be a better choice. I've moved the files there. With that change, diff --git a/packages/block-editor/src/components/block-list/index.native.js b/packages/block-editor/src/components/block-list/index.native.js
index 810e23e4c1..b4d69aa7c7 100644
--- a/packages/block-editor/src/components/block-list/index.native.js
+++ b/packages/block-editor/src/components/block-list/index.native.js
@@ -30,6 +30,7 @@ import {
import { BlockDraggableWrapper } from '../block-draggable';
import { useEditorWrapperStyles } from '../../hooks/use-editor-wrapper-styles';
import { store as blockEditorStore } from '../../store';
+import OfflineStatus from '../offline-status';
const identity = ( x ) => x;
@@ -235,6 +236,7 @@ export default function BlockList( {
onLayout={ onLayout }
testID="block-list-wrapper"
>
+ { __DEV__ && <OfflineStatus /> }
{ isRootList ? (
<BlockListProvider
value={ { |
I think this is a good approach. I think we can begin merging the work in #56609 and associated PRs. Once merged, we can follow the rebasing steps you've outlined, and update WDYT? Do you note any other code changes needed in this PR (outside of depending upon the #56609 merge flow)? |
derekblank
commented
Dec 6, 2023
| @@ -11,6 +11,7 @@ For each user feature we should also add a importance categorization label to i | |||
|
|
|||
| ## Unreleased | |||
| - [*] [internal] Move InserterButton from components package to block-editor package [#56494] | |||
| - [**] Editor displays network status when offline [#56627] | |||
There was a problem hiding this comment.
If we were to merge this PR within the next week with this work behind a __DEV__ flag, this may not be the most accurate release note as it would not display network status until connected to the connectivity hook. Do we have a precedent for release notes with dev-only changes? Perhaps just [internal]?
There was a problem hiding this comment.
I believe we have generally merely excluded release notes until the user-facing features are present. I.e. we would add the release note in the PR removing the __DEV__ flag.
That said, changing this release note to be an [internal] note more focused on the implementation details would likely be fine as well, e.g. [internal] Add OfflineStatus component for communicating network connection state.
derekblank
commented
Dec 6, 2023
| ); | ||
|
|
||
| const iconStyle = usePreferredColorSchemeStyle( | ||
| styles.icon, |
There was a problem hiding this comment.
Noting that the linter will throw an error if these are written in square-bracket syntax, e.g., styles[ 'icon' ]:
21:11 error ["container"] is better written in dot notation dot-notation
26:11 error ["text"] is better written in dot notation dot-notation
31:11 error ["icon"] is better written in dot notation dot-notation
Perhaps a quirk of BEM semantics, but using dot-notation for light styles and square-bracket syntax for dark styles, unless you have any suggestions.
There was a problem hiding this comment.
Your approach is expected and correct. For better or worse, we only use bracket notations for selectors that include hyphens.
derekblank
changed the title
dcalhoun
reviewed
Dec 6, 2023
There was a problem hiding this comment.
Thanks for making the changes. 🙇🏻
I noticed the contrast between the offline banner and top navigation header differs when comparing light and dark mode. The former uses different background colors for each element, the latter uses the same background color. A small detail, but I was unsure if the divergence was intentional.
Appearance Divergence
| Light Mode | Dark Mode |
|---|---|
 |
 |
I think this [stacking branches] is a good approach. I think we can begin merging the work in #56609 and associated PRs.
To clarify, the example git rebase --onto command I shared is only relevant if we rebased this branch atop the dependent feat/connectivity-status before it is merged into the trunk branch.
Stacked Branches Details
git switch rnmobile/offline-statusgit pull --rebase origin feat/connectivity-status- Integrate the
isConnectedhook into theOfflineStatuscomponent. git push origin rnmobile/offline-status --force-with-lease- Update thie PR to target
feat/connectivity-statusinstead oftrunk. - Merge
feat/connectivity-statusintotrunk. Note: GitHub will now automatically update this PR to targettrunkinstead offeat/connectivity-status. - Temporarily restore the
feat/connected-statusbranch in GitHub. git fetchgit rebase --onto origin/trunk feat/connected-status rnmobile/offline-statusgit push origin rnmobile/offline-status --force-with-lease
Steps 1–5 would've been done originally when starting this work, building atop the dependent branch. Steps 6–10 would be done to rebase this branch atop trunk after the merge.
Now that we are likely about to merge feat/connectivity-status into trunk, this approach is less relevant for this particular PR.
If your plan is to await merging feat/connectivity-status before rebasing, then we can simply rebase this branch on trunk after the merge and work to integrate the newly available isConnected hook into this branch.
Await Dependent Work Merge Details
- Merge
feat/connectivity-statusintotrunk. git switch rnmobile/offline-statusgit pull --rebase origin trunk- Integrate the
isConnectedhook into theOfflineStatuscomponent. git push origin rnmobile/offline-status --force-with-lease
We can now review the changes in this PR with the isConnected hook integrated.
[...] Once merged, we can follow the rebasing steps you've outlined, and update
block-listto something like this to have the OfflineStatus component available behind a dev flag in order to merge this PR:{ __DEV__ && ! isConnected && <OfflineStatus /> }
My thought was that we would use isConnected Hook within the OfflineStatus component, returning null when isConnected is true. This would keep the network state closer to the component that uses it. It might also avoid unnecessary re-renders of block-list — not sure. WDYT?
| ); | ||
|
|
||
| const iconStyle = usePreferredColorSchemeStyle( | ||
| styles.icon, |
There was a problem hiding this comment.
Your approach is expected and correct. For better or worse, we only use bracket notations for selectors that include hyphens.
| @@ -11,6 +11,7 @@ For each user feature we should also add a importance categorization label to i | |||
|
|
|||
| ## Unreleased | |||
| - [*] [internal] Move InserterButton from components package to block-editor package [#56494] | |||
| - [**] Editor displays network status when offline [#56627] | |||
There was a problem hiding this comment.
I believe we have generally merely excluded release notes until the user-facing features are present. I.e. we would add the release note in the PR removing the __DEV__ flag.
That said, changing this release note to be an [internal] note more focused on the implementation details would likely be fine as well, e.g. [internal] Add OfflineStatus component for communicating network connection state.
I agree that using Noting that the merge of feat/connectivity-status was reverted. I plan to continue using the Stacked Branches Details approach. |
* Remove dropdown menu * Make sure setEditorCanvasContainerView() calls are made when needed so that toggle status works correctly when viewing any other sections of global styles. * Pre select the revisions count to prevent flashes of incorrect revision counts. Hat tip @ramonjd. * Remove CSS no longer needed. * Modify e2e test to reflect removal of menu item. * Fix issue with style book closing when browsing blocks. * Replace onClick with onBack to reduce confusion with screen header click action. * Change ordering of `isRevisionsOpened` const.
* Add kebab casing and font family wrapping in sanitization function. Remove commented code. * Simplify how kebab casing and quote wrapping is done. * Ensure incoming font is returned. * Add kebab casing to slug in form data. * Add tests for wpKebabCase * remove redundant test * test 2 different inputs on first wpKebabCase case * Move formatting functions to utils. * reduce the scope of the function * fix the live preview (without reloading the page) of the just installed fonts in the editor * format php * Add test for format_font_family. * Format php. * Add more test cases and format. * Remove accidental badKey. * Format. --------- Co-authored-by: Vicente Canales <vicente.canales@automattic.com> Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com> Co-authored-by: Sarah Norris <sarah@sekai.co.uk> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>
…hiteSpace()` if `value` is not a string (#56570) * (RichText)(Fix)(Workaround) Make sure value is always a string to avoid TypeErorrs See the comments in this changeset for more info on why this was needed. * Use a more localized fix that does not change the argument
* Add setting to disable custom content size controls * Make sure theme setting overrides block setting. * Revert "Make sure theme setting overrides block setting." This reverts commit ad4b76c. * Change setting name to `allowCustomContentAndWideSize`
…6334) * Add block registration documentation * Update block registration process * Update block registration in client code * updated doc * Update block registration settings * Grammar check * Moved Registration of A block to fundamentals of block development * Added maarkdown extension * Delete docs/getting-started/fundamentals-block-development/registration-of-a-block * Removed "key" * clarification path block.json * Fix block.json registration path * server-side features require block server registration * server-side features require block server registration * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update docs/getting-started/fundamentals-block-development/registration-of-a-block.md Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Fix formatting in registration-of-a-block.md * Added link to new page and some text adjustments * Update block registration function and build process reference --------- Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
…the Block Editor (#56553) * Add JavaScript build process documentation * Update docs/getting-started/fundamentals-block-development/javascript-in-the-block-editor.md Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de> --------- Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
…56551) * Add file structure documentation for custom blocks * Update docs/getting-started/fundamentals-block-development/file-structure-of-a-block.md Co-authored-by: Ryan Welcher <me@ryanwelcher.com> * Removed not clear sentence --------- Co-authored-by: Ryan Welcher <me@ryanwelcher.com>
derekblank
removed request for
noisysocks,
talldan,
gziolo,
swissspidy,
ntwb,
ajitbohra,
mmtr,
ryanwelcher,
nerrad,
adamsilverstein,
luisherranz,
ellatrix,
ndiego,
geriux,
ajlende,
artemiomorales,
michalczaplinski,
dmsnell,
noahtallen,
ObliviousHarmony,
DAreRodz,
kevin940726,
tellthemachines,
alexstine,
fluiddot,
fabiankaegy and
t-hamano
|
Closing this PR due to the rebase complexity. Will reopen another one based on |
|
Thought for wherever this is resurrected, but the browser API also exposes online/offline events. I'm not sure if this already incorporates that element, but there's no reason we need to limit the status indicator to mobile devices. |
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.
What?
Adds a mobile component to display a visual indicator when working offline within the mobile editor. (Note: the functionality to subscribe to network status will be handled in a separate PR.)
Why?
Overall, to improve the user's experience when uploading media.
Addresses:
How?
Adds OfflineStatus component and updates Layout component to host it.
Testing Instructions
Applying the test patch will display the component statically until it is wired to the connectivity subscription hook proposed in #56609.
Test Diff
Screenshots or screencast