Skip to content

Commit

Permalink
feat: Update Image block failed upload visuals (#56937)
Browse files Browse the repository at this point in the history
* refactor: Rename `useIsConnected` to `useNetworkConnectivity`

Attempt to better communicate the Hook intent.

* test: Add `useNetworkConnectivity` Hook tests

* docs: Document `useNetworkConnectivity` Hook

* refactor: Rename `withIsConnected` to `withNetworkConnectivity`

* fix: Optimistically presume network connectivity

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.

* test: Create realistic default `requestConnectionStatus` mock

* fix: Prevent duplicative offline status indicators

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.

* Add subscribeConnectionStatus to MediaUploadProgress component

* Update MediaUploadProgress to use withIsConnected HOC

* refactor: Fix HoC name reference

This was renamed in a commit onto which this branch was rebased.

* refactor: Enable Fast Refresh for Image component

Place non-React component exports in a separate module to enable Fast
Refresh of the React component.

* feat: Update the image offline message

Match the latest design comps.

* refactor: Avoid unnecessary inconsistency in switch case logic

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.

* refactor: Enable Fast Refresh for Image component

Place non-React component exports in a separate module to enable Fast
Refresh of the React component.

* feat: Revert logic intended to display progress bar when offline

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.

* refactor: Remove sizing from offline SVG

This allows sizing the icon for different contexts.

* feat: Update Image block media upload visuals

Update iconography and messaging when offline or or there is a upload
error.

* feat: Add paused media upload state

Relying upon the network connection status alone results in the error
message erroneously changing whenever a network connection is
re-established. The error message, in fact, is a state communicated from
the host app's attempt to upload media. Therefore, this creates a new
pause state to represent uploads paused while offline.

* feat: Scope the paused upload state to the Image block

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.

* feat: Enable paused media uploads for Media and Text block types

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.

* perf: Avoid unnecessary state updates and callback invocations

If the upload state and progress is identical, there should be no reason
to invoke callbacks, update component state, or re-render.

* Add MEDIA_UPLOAD_STATE_PAUSED to Android DeferredEvent emitter

* feat: Add Android paused media upload bridge methods

Enables the Android host app to mirror the functionality of iOS.

* docs: Add change log entry

* feat: Add image upload failure dark mode styles

---------

Co-authored-by: David Calhoun <[email protected]>
  • Loading branch information
derekblank and dcalhoun authored Jan 9, 2024
1 parent e27b306 commit ffa9035
Show file tree
Hide file tree
Showing 16 changed files with 189 additions and 54 deletions.
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ export {
MEDIA_TYPE_AUDIO,
MEDIA_TYPE_ANY,
} from './media-upload/constants';
export { default as MediaUploadProgress } from './media-upload-progress';
export {
default as MediaUploadProgress,
MEDIA_UPLOAD_STATE_UPLOADING,
MEDIA_UPLOAD_STATE_SUCCEEDED,
MEDIA_UPLOAD_STATE_FAILED,
MEDIA_UPLOAD_STATE_RESET,
} from './media-upload-progress';
} from './media-upload-progress/constants';
export { default as BlockMediaUpdateProgress } from './block-media-update-progress';
export { default as URLInput } from './url-input';
export { default as BlockInvalidWarning } from './block-list/block-invalid-warning';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export const MEDIA_UPLOAD_STATE_IDLE = 0;
export const MEDIA_UPLOAD_STATE_UPLOADING = 1;
export const MEDIA_UPLOAD_STATE_SUCCEEDED = 2;
export const MEDIA_UPLOAD_STATE_FAILED = 3;
export const MEDIA_UPLOAD_STATE_RESET = 4;
export const MEDIA_UPLOAD_STATE_PAUSED = 11;
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,28 @@ import { subscribeMediaUpload } from '@wordpress/react-native-bridge';
* Internal dependencies
*/
import styles from './styles.scss';

export const MEDIA_UPLOAD_STATE_UPLOADING = 1;
export const MEDIA_UPLOAD_STATE_SUCCEEDED = 2;
export const MEDIA_UPLOAD_STATE_FAILED = 3;
export const MEDIA_UPLOAD_STATE_RESET = 4;
import {
MEDIA_UPLOAD_STATE_IDLE,
MEDIA_UPLOAD_STATE_UPLOADING,
MEDIA_UPLOAD_STATE_SUCCEEDED,
MEDIA_UPLOAD_STATE_FAILED,
MEDIA_UPLOAD_STATE_RESET,
MEDIA_UPLOAD_STATE_PAUSED,
} from './constants';

export class MediaUploadProgress extends Component {
constructor( props ) {
super( props );

this.state = {
uploadState: MEDIA_UPLOAD_STATE_IDLE,
progress: 0,
isUploadInProgress: false,
isUploadFailed: false,
};

this.mediaUpload = this.mediaUpload.bind( this );
this.getRetryMessage = this.getRetryMessage.bind( this );
}

componentDidMount() {
Expand All @@ -45,7 +50,11 @@ export class MediaUploadProgress extends Component {
mediaUpload( payload ) {
const { mediaId } = this.props;

if ( payload.mediaId !== mediaId ) {
if (
payload.mediaId !== mediaId ||
( payload.state === this.state.uploadState &&
payload.progress === this.state.progress )
) {
return;
}

Expand All @@ -56,6 +65,9 @@ export class MediaUploadProgress extends Component {
case MEDIA_UPLOAD_STATE_SUCCEEDED:
this.finishMediaUploadWithSuccess( payload );
break;
case MEDIA_UPLOAD_STATE_PAUSED:
this.finishMediaUploadWithPause( payload );
break;
case MEDIA_UPLOAD_STATE_FAILED:
this.finishMediaUploadWithFailure( payload );
break;
Expand All @@ -68,6 +80,7 @@ export class MediaUploadProgress extends Component {
updateMediaProgress( payload ) {
this.setState( {
progress: payload.progress,
uploadState: payload.state,
isUploadInProgress: true,
isUploadFailed: false,
} );
Expand All @@ -77,21 +90,48 @@ export class MediaUploadProgress extends Component {
}

finishMediaUploadWithSuccess( payload ) {
this.setState( { isUploadInProgress: false } );
this.setState( {
uploadState: payload.state,
isUploadInProgress: false,
} );
if ( this.props.onFinishMediaUploadWithSuccess ) {
this.props.onFinishMediaUploadWithSuccess( payload );
}
}

finishMediaUploadWithPause( payload ) {
if ( ! this.props.enablePausedUploads ) {
this.finishMediaUploadWithFailure( payload );
return;
}

this.setState( {
uploadState: payload.state,
isUploadInProgress: true,
isUploadFailed: false,
} );
if ( this.props.onFinishMediaUploadWithFailure ) {
this.props.onFinishMediaUploadWithFailure( payload );
}
}

finishMediaUploadWithFailure( payload ) {
this.setState( { isUploadInProgress: false, isUploadFailed: true } );
this.setState( {
uploadState: payload.state,
isUploadInProgress: false,
isUploadFailed: true,
} );
if ( this.props.onFinishMediaUploadWithFailure ) {
this.props.onFinishMediaUploadWithFailure( payload );
}
}

mediaUploadStateReset( payload ) {
this.setState( { isUploadInProgress: false, isUploadFailed: false } );
this.setState( {
uploadState: payload.state,
isUploadInProgress: false,
isUploadFailed: false,
} );
if ( this.props.onMediaUploadStateReset ) {
this.props.onMediaUploadStateReset( payload );
}
Expand All @@ -115,15 +155,24 @@ export class MediaUploadProgress extends Component {
}
}

getRetryMessage() {
if (
this.state.uploadState === MEDIA_UPLOAD_STATE_PAUSED &&
this.props.enablePausedUploads
) {
return __( 'Waiting for connection' );
}

// eslint-disable-next-line @wordpress/i18n-no-collapsible-whitespace
return __( 'Failed to insert media.\nTap for more info.' );
}

render() {
const { renderContent = () => null } = this.props;
const { isUploadInProgress, isUploadFailed } = this.state;
const { isUploadInProgress, isUploadFailed, uploadState } = this.state;
const showSpinner = this.state.isUploadInProgress;
const progress = this.state.progress * 100;
// eslint-disable-next-line @wordpress/i18n-no-collapsible-whitespace
const retryMessage = __(
'Failed to insert media.\nTap for more info.'
);
const retryMessage = this.getRetryMessage();

const progressBarStyle = [
styles.progressBar,
Expand All @@ -149,6 +198,9 @@ export class MediaUploadProgress extends Component {
) }
</View>
{ renderContent( {
isUploadPaused:
uploadState === MEDIA_UPLOAD_STATE_PAUSED &&
this.props.enablePausedUploads,
isUploadInProgress,
isUploadFailed,
retryMessage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import {
/**
* Internal dependencies
*/
import { MediaUploadProgress } from '../';
import {
MediaUploadProgress,
MEDIA_UPLOAD_STATE_UPLOADING,
MEDIA_UPLOAD_STATE_SUCCEEDED,
MEDIA_UPLOAD_STATE_FAILED,
MEDIA_UPLOAD_STATE_RESET,
} from '../';
} from '../constants';

let uploadCallBack;
subscribeMediaUpload.mockImplementation( ( callback ) => {
Expand Down
3 changes: 3 additions & 0 deletions packages/block-library/src/image/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,7 @@ export class ImageEdit extends Component {
{ ! this.state.isCaptionSelected &&
getToolbarEditButton( openMediaOptions ) }
<MediaUploadProgress
enablePausedUploads
coverUrl={ url }
mediaId={ id }
onUpdateMediaProgress={ this.updateMediaProgress }
Expand All @@ -825,6 +826,7 @@ export class ImageEdit extends Component {
this.mediaUploadStateReset
}
renderContent={ ( {
isUploadPaused,
isUploadInProgress,
isUploadFailed,
retryMessage,
Expand All @@ -843,6 +845,7 @@ export class ImageEdit extends Component {
! isCaptionSelected
}
isUploadFailed={ isUploadFailed }
isUploadPaused={ isUploadPaused }
isUploadInProgress={
isUploadInProgress
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class MediaContainer extends Component {
mediaWidth,
shouldStack,
} = this.props;
const { isUploadFailed, retryMessage } = params;
const { isUploadFailed, isUploadPaused, retryMessage } = params;
const focalPointValues = ! focalPoint
? IMAGE_DEFAULT_FOCAL_POINT
: focalPoint;
Expand Down Expand Up @@ -203,6 +203,7 @@ class MediaContainer extends Component {
focalPoint={ imageFill && focalPointValues }
isSelected={ isMediaSelected }
isUploadFailed={ isUploadFailed }
isUploadPaused={ isUploadPaused }
isUploadInProgress={ isUploadInProgress }
onSelectMediaUploadOption={
this.onSelectMediaUploadOption
Expand Down Expand Up @@ -340,6 +341,7 @@ class MediaContainer extends Component {
{ getMediaOptions() }

<MediaUploadProgress
enablePausedUploads
coverUrl={ coverUrl }
mediaId={ mediaId }
onUpdateMediaProgress={
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ export { default as LinkSettings } from './mobile/link-settings';
export { default as LinkSettingsScreen } from './mobile/link-settings/link-settings-screen';
export { default as LinkSettingsNavigation } from './mobile/link-settings/link-settings-navigation';
export { default as SegmentedControl } from './mobile/segmented-control';
export { default as Image, IMAGE_DEFAULT_FOCAL_POINT } from './mobile/image';
export { default as Image } from './mobile/image';
export { IMAGE_DEFAULT_FOCAL_POINT } from './mobile/image/constants';
export { default as ImageEditingButton } from './mobile/image/image-editing-button';
export { setClipboard, getClipboard } from './mobile/clipboard';
export { default as AudioPlayer } from './mobile/audio-player';
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/mobile/image/constants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const IMAGE_DEFAULT_FOCAL_POINT = { x: 0.5, y: 0.5 };
Loading

0 comments on commit ffa9035

Please sign in to comment.