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.

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

[RNMobile] Refactor Caption component #19118

Merged
merged 6 commits into from
Jan 13, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
65 changes: 65 additions & 0 deletions packages/block-editor/src/components/block-caption/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* External dependencies
*/
import { View } from 'react-native';

/**
* WordPress dependencies
*/
import { Caption, RichText } from '@wordpress/block-editor';
import { compose } from '@wordpress/compose';
import { withDispatch, withSelect } from '@wordpress/data';

const BlockCaption = ( {
accessible,
accessibilityLabelCreator,
onBlur,
onChange,
onFocus,
isSelected,
shouldDisplay,
text,
} ) => (
<View
style={ { flex: 1, padding: 12 } }
>
<Caption
accessibilityLabelCreator={ accessibilityLabelCreator }
accessible={ accessible }
isSelected={ isSelected }
onBlur={ onBlur }
onChange={ onChange }
onFocus={ onFocus }
shouldDisplay={ shouldDisplay }
value={ text }
/>
</View>
);

export default compose( [
withSelect( ( select, { clientId } ) => {
const {
getBlockAttributes,
getSelectedBlockClientId,
} = select( 'core/block-editor' );
const { caption } = getBlockAttributes( clientId );
const isBlockSelected = getSelectedBlockClientId() === clientId;

// We'll render the caption so that the soft keyboard is not forced to close on Android
// but still hide it by setting its display style to none. See wordpress-mobile/gutenberg-mobile#1221
const shouldDisplay = ! RichText.isEmpty( caption ) > 0 || isBlockSelected;

return {
shouldDisplay,
text: caption,
};
} ),
withDispatch( ( dispatch, { clientId } ) => {
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
return {
onChange: ( caption ) => {
updateBlockAttributes( clientId, { caption } );
},
};
} ),
] )( BlockCaption );
86 changes: 40 additions & 46 deletions packages/block-editor/src/components/caption/index.native.js
Original file line number Diff line number Diff line change
@@ -1,65 +1,59 @@
/**
* External dependencies
*/
import { View } from 'react-native';
import { Platform, View } from 'react-native';

/**
* WordPress dependencies
*/
import { RichText } from '@wordpress/block-editor';
import { compose } from '@wordpress/compose';
import { withDispatch, withSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

const Caption = ( { accessible, accessibilityLabel, onBlur, onChange, onFocus, isSelected, shouldDisplay, text } ) => (
<View style={ { padding: 12, flex: 1, display: shouldDisplay ? 'flex' : 'none' } }
// this platform difference is needed to avoid a regression described here:
// https://github.com/WordPress/gutenberg/pull/18818#issuecomment-559818548
const CAPTION_TAG_NAME = Platform.select( {
ios: 'p',
android: '',
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

This was in the Gallery block to make it work correctly on Android due to the mentioned regression so I moved it to the Caption component itself since it was affecting the rest of Blocks using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Linking this up with the issue that was filed: wordpress-mobile/gutenberg-mobile#1651

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this platform divergence is consolidated into one place, OTOH, it seems we currently have to "choose" between two regressions for all captions: Enter key not working or First word not style-able. This PR does not introduce these issues, but we do lose some granularity in how we can "choose which regression" 🙈 for the different use cases. I'm not suggesting a change here, just noting, and hopefully those issues will be resolved, and this will soon be a moot point. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right, by moving this to the Caption component it will affect Block level components with the First word not style-able regression, but it will allow users to add new lines on Android... I wonder what users use the most, styling in captions or entering new lines 😅


const Caption = ( {
accessibilityLabelCreator,
accessible,
inlineToolbar,
isSelected,
onBlur,
onChange,
onFocus,
placeholder = __( 'Write caption…' ),
placeholderTextColor,
shouldDisplay = true,
style,
value,
} ) => (
<View
accessibilityLabel={ accessibilityLabelCreator ? accessibilityLabelCreator( value ) : undefined }
accessibilityRole="button"
accessible={ accessible }
accessibilityLabel={ accessibilityLabel }
accessibilityRole={ 'button' }
style={ { flex: 1, display: shouldDisplay ? 'flex' : 'none' } }
>
<RichText
rootTagsToEliminate={ [ 'p' ] }
placeholder={ __( 'Write caption…' ) }
value={ text }
onChange={ onChange }
unstableOnFocus={ onFocus }
onBlur={ onBlur } // always assign onBlur as props
isSelected={ isSelected }
__unstableMobileNoFocusOnMount
fontSize={ 14 }
fontSize={ style && style.fontSize ? style.fontSize : 14 }
inlineToolbar={ inlineToolbar }
isSelected={ isSelected }
onBlur={ onBlur }
onChange={ onChange }
placeholder={ placeholder }
placeholderTextColor={ placeholderTextColor }
rootTagsToEliminate={ [ 'p' ] }
style={ style }
tagName={ CAPTION_TAG_NAME }
textAlign="center"
underlineColorAndroid="transparent"
textAlign={ 'center' }
tagName={ 'p' }
unstableOnFocus={ onFocus }
value={ value }
/>
</View>
);

export default compose( [
withSelect( ( select, { accessibilityLabelCreator, clientId } ) => {
const {
getBlockAttributes,
getSelectedBlockClientId,
} = select( 'core/block-editor' );
const { caption } = getBlockAttributes( clientId );
const accessibilityLabel = accessibilityLabelCreator ? accessibilityLabelCreator( caption ) : undefined;
const isBlockSelected = getSelectedBlockClientId() === clientId;

// We'll render the caption so that the soft keyboard is not forced to close on Android
// but still hide it by setting its display style to none. See wordpress-mobile/gutenberg-mobile#1221
const shouldDisplay = ! RichText.isEmpty( caption ) > 0 || isBlockSelected;

return {
accessibilityLabel,
shouldDisplay,
text: caption,
};
} ),
withDispatch( ( dispatch, { clientId } ) => {
const { updateBlockAttributes } = dispatch( 'core/block-editor' );
return {
onChange: ( caption ) => {
updateBlockAttributes( clientId, { caption } );
},
};
} ),
] )( Caption );
export default Caption;
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ export { default as MediaUpload, MEDIA_TYPE_IMAGE, MEDIA_TYPE_VIDEO } from './me
export { default as MediaUploadProgress } from './media-upload-progress';
export { default as URLInput } from './url-input';
export { default as BlockInvalidWarning } from './block-list/block-invalid-warning';
export { default as BlockCaption } from './block-caption';
export { default as Caption } from './caption';

export { BottomSheetSettings, BlockSettingsButton } from './block-settings';
export { default as VideoPlayer, VIDEO_ASPECT_RATIO } from './video-player';

Expand Down
39 changes: 17 additions & 22 deletions packages/block-library/src/gallery/gallery-image.native.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { Image, StyleSheet, View, ScrollView, Text, TouchableWithoutFeedback, Platform } from 'react-native';
import { Image, StyleSheet, View, ScrollView, Text, TouchableWithoutFeedback } from 'react-native';
import {
requestImageFailedRetryDialog,
requestImageUploadCancelDialog,
Expand All @@ -14,7 +14,7 @@ import { isEmpty } from 'lodash';
import { Component } from '@wordpress/element';
import { Icon } from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { RichText, MediaUploadProgress } from '@wordpress/block-editor';
import { Caption, MediaUploadProgress } from '@wordpress/block-editor';
import { isURL } from '@wordpress/url';
import { withPreferredColorScheme } from '@wordpress/compose';

Expand All @@ -32,20 +32,14 @@ const removeButtonStyle = compose( style.removeButton, { aspectRatio: 1 } );
const ICON_SIZE_ARROW = 15;
const ICON_SIZE_REMOVE = 20;

// this platform difference is needed to avoid a regression described here:
// https://github.com/WordPress/gutenberg/pull/18818#issuecomment-559818548
const CAPTION_TAG_NAME = Platform.select( {
ios: 'p',
android: '',
} );

class GalleryImage extends Component {
constructor() {
super( ...arguments );

this.onSelectImage = this.onSelectImage.bind( this );
this.onSelectCaption = this.onSelectCaption.bind( this );
this.onMediaPressed = this.onMediaPressed.bind( this );
this.onCaptionChange = this.onCaptionChange.bind( this );
this.bindContainer = this.bindContainer.bind( this );

this.updateMediaProgress = this.updateMediaProgress.bind( this );
Expand Down Expand Up @@ -104,6 +98,11 @@ class GalleryImage extends Component {
}
}

onCaptionChange( caption ) {
const { setAttributes } = this.props;
setAttributes( { caption } );
}

componentDidUpdate( prevProps ) {
const { isSelected, image, url } = this.props;
if ( image && ! url ) {
Expand Down Expand Up @@ -150,10 +149,10 @@ class GalleryImage extends Component {
renderContent( params ) {
const {
url, isFirstItem, isLastItem, isSelected, caption, onRemove,
onMoveForward, onMoveBackward, setAttributes, 'aria-label': ariaLabel,
onMoveForward, onMoveBackward, 'aria-label': ariaLabel,
isCropped, getStylesFromColorScheme } = this.props;

const { isUploadInProgress } = this.state;
const { isUploadInProgress, captionSelected } = this.state;
const { isUploadFailed, retryMessage } = params;
const resizeMode = isCropped ? 'cover' : 'contain';

Expand Down Expand Up @@ -234,19 +233,15 @@ class GalleryImage extends Component {
{ ( shouldShowCaptionEditable || shouldShowCaptionExpanded ) && (
<View style={ captionContainerStyle }>
<ScrollView nestedScrollEnabled keyboardShouldPersistTaps="handled">
<RichText
tagName={ CAPTION_TAG_NAME }
rootTagsToEliminate={ [ 'p' ] }
<Caption
inlineToolbar
isSelected={ captionSelected }
onChange={ this.onCaptionChange }
onFocus={ this.onSelectCaption }
placeholder={ isSelected ? __( 'Write caption…' ) : null }
value={ caption }
isSelected={ this.state.captionSelected }
onChange={ ( newCaption ) => setAttributes( { caption: newCaption } ) }
unstableOnFocus={ this.onSelectCaption }
fontSize={ captionStyle.fontSize }
style={ captionStyle }
textAlign={ captionStyle.textAlign }
placeholderTextColor={ captionPlaceholderStyle.color }
inlineToolbar
style={ captionStyle }
value={ caption }
/>
</ScrollView>
</View>
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/gallery/gallery.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import Tiles from './tiles';
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { Caption } from '@wordpress/block-editor';
import { BlockCaption } from '@wordpress/block-editor';
import { useState } from '@wordpress/element';

const TILE_SPACING = 15;
Expand Down Expand Up @@ -109,7 +109,7 @@ export const Gallery = ( props ) => {
} ) }
</Tiles>
{ mediaPlaceholder }
<Caption
<BlockCaption
clientId={ clientId }
isSelected={ isCaptionSelected }
accessible={ true }
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/image/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
} from '@wordpress/components';

import {
Caption,
BlockCaption,
MediaPlaceholder,
MediaUpload,
MediaUploadProgress,
Expand Down Expand Up @@ -450,7 +450,7 @@ export class ImageEdit extends React.Component {
);
} }
/>
<Caption
<BlockCaption
clientId={ this.props.clientId }
isSelected={ this.state.isCaptionSelected }
accessible={ true }
Expand Down
24 changes: 20 additions & 4 deletions packages/block-library/src/video/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
*/
import React from 'react';
import { View, TouchableWithoutFeedback, Text } from 'react-native';
import { isEmpty } from 'lodash';

/**
* Internal dependencies
*/
Expand All @@ -23,7 +25,7 @@ import {
} from '@wordpress/components';
import { withPreferredColorScheme } from '@wordpress/compose';
import {
Caption,
BlockCaption,
MediaPlaceholder,
MediaUpload,
MediaUploadProgress,
Expand All @@ -33,7 +35,7 @@ import {
VideoPlayer,
InspectorControls,
} from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import { isURL } from '@wordpress/url';
import { doAction, hasAction } from '@wordpress/hooks';

Expand Down Expand Up @@ -196,7 +198,11 @@ class VideoEdit extends React.Component {
}

return (
<TouchableWithoutFeedback onPress={ this.onVideoPressed } disabled={ ! isSelected }>
<TouchableWithoutFeedback
accessible={ ! isSelected }
onPress={ this.onVideoPressed }
disabled={ ! isSelected }
>
<View style={ { flex: 1 } }>
{ ! this.state.isCaptionSelected &&
<BlockControls>
Expand Down Expand Up @@ -256,7 +262,17 @@ class VideoEdit extends React.Component {
);
} }
/>
<Caption
<BlockCaption
accessible={ true }
accessibilityLabelCreator={ ( caption ) =>
isEmpty( caption ) ?
/* translators: accessibility text. Empty video caption. */
( 'Video caption. Empty' ) :
sprintf(
/* translators: accessibility text. %s: video caption. */
__( 'Video caption. %s' ),
caption )
}
clientId={ this.props.clientId }
isSelected={ this.state.isCaptionSelected }
onFocus={ this.onFocusCaption }
Expand Down