feat: Update Image block failed upload visuals #56937
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
derekblank
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 40b90b1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7201472144
|
dcalhoun
reviewed
Dec 11, 2023
packages/block-editor/src/components/media-upload-progress/index.native.js
Outdated
Show resolved
Hide resolved
derekblank
commented
Dec 12, 2023
packages/block-editor/src/components/media-upload-progress/index.native.js
Outdated
Show resolved
Hide resolved
derekblank
commented
Dec 12, 2023
packages/block-editor/src/components/media-upload-progress/index.native.js
Outdated
Show resolved
Hide resolved
derekblank
changed the title
dcalhoun
force-pushed
the
rnmobile/media-upload-progress-connection-subscription
branch
from
5fe51e1
to
296c53b
Compare
dcalhoun
reviewed
Dec 13, 2023
packages/block-editor/src/components/media-upload-progress/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-upload-progress/index.native.js
Outdated
Show resolved
Hide resolved
dcalhoun
force-pushed
the
rnmobile/media-upload-progress-connection-subscription
branch
from
296c53b
to
40b90b1
Compare
Attempt to better communicate the Hook intent.
Prior to making the asynchronous request to the host app across the bridge, it is a better UX to presume network connectivity is present rather than displaying network connectivity messages briefly.
Hoist the `OfflineStatus` indicator from the block list to the editor. The block list is leveraged for inner blocks, which means it rendered nested `OfflineStatus` indicators for blocks with inner blocks. Additionally, the `editor` package feels like an appropriate location for the offline detection component.
This was renamed in a commit onto which this branch was rebased.
Place non-React component exports in a separate module to enable Fast Refresh of the React component.
Match the latest design comps.
Replace early return with logic mirroring the rest of the switch case statements. While I suppose it could be considered subjective, it is odd and confusing that one of the three cases was structured differently.
Place non-React component exports in a separate module to enable Fast Refresh of the React component.
I believe this logic erroneously prevented the spinner progress bar from displaying when an image is successfully uploading. I.e., the logic read "if show spinner and online, apply 'progress bar hidden' styles." I imagine this confusion is a result of the previous logic using the `||` pattern to conditionally apply the hidden styles, which is confusing in itself. This logic should likely be implemented by the way the host app communicates the upload state. Implementing this in the JavaScript logic is confusing or misleading, in my opinion. It adds a new, somewhat surprising interpretation of the existing states provided by the host app.
This allows sizing the icon for different contexts.
Update iconography and messaging when offline or or there is a upload error.
This looks like a good lead for iOS. I haven't precisely identified where the same logic occurs in Android, but onMediaSaveFailed may be a good place to start. It may also come from onMediaUploadFailed in Aztec, but this seems less likely. Will investigate further.
Indeed. While we can modify the style code so the progress bar persists when offline, the progress value itself resets to 0, so only the progress bar track is displayed. We could possibly modify this code to hook into the network connectivity state to not mark a media upload as failed when offline. This may be the most complex part of code remaining to resolve. |
I identified further the Android code areas we'd need to update in #57476. |
The planned scope of the project was to improve the upload experience of the Image block, as it is the most frequently used media block type. Support for additional block types could be added at a later time.
Because the Media and Text block type relies upon a nested Image block, it makes sense to enable the paused media uploads for a consistent UX.
If the upload state and progress is identical, there should be no reason to invoke callbacks, update component state, or re-render.
Merged
13 tasks
Enables the Android host app to mirror the functionality of iOS.
12 tasks
…media-upload-progress-connection-subscription
dcalhoun
approved these changes
Jan 4, 2024
|
@derekblank I cannot request your review on this PR as you are the original author, but I believe it would be worthwhile for you to perform the final review of the code please. |
|
@dcalhoun For some reason, it is not letting me add myself as a reviewer, even if I assign myself to the PR -- my name simply doesn't appear in the potential reviewers list. Perhaps because I opened it, and you approved already? Anyways, I tested and reviewed a final time -- LGTM. 🚀 |
dcalhoun
deleted the
rnmobile/media-upload-progress-connection-subscription
branch
4 tasks
4 tasks
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 PRs
What?
Display a custom message for failed media uploads that are paused due to the lack of an internet connection.
Why?
Fixes wordpress-mobile/gutenberg-mobile#6407. Improve the UX by better communicating to users why the media upload is failed so that they may take action.
How?
Integrate a new "paused" media upload state that is sent by the host app. For block using
enablePausedUploads(currently, only Image blocks), a specific "Waiting for connection" error message is displayed.Testing Instructions
Testing Instructions for Keyboard
Attempt to complete the aforementioned testing instructions via a mobile device's screen reader.
Screenshots or screencast
iOS
Android