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.

Sign up for GitHub

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

[Mobile]: Add Image Caption Styling #14883

Merged
merged 13 commits into from
Apr 27, 2019
Merged

[Mobile]: Add Image Caption Styling #14883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ab9dab2
Add ability to style image caption
mchowning Apr 8, 2019
b597c6e
Address style errors from linter
mchowning Apr 8, 2019
b4afb1d
Hide image toolbar controls if caption selected
mchowning Apr 8, 2019
a738017
Update state in componentDidMount
mchowning Apr 9, 2019
5d3f86e
Add comment explaining isSelected guard
mchowning Apr 11, 2019
ccf785c
Merge remote-tracking branch 'origin/master' into issue/image_caption…
mchowning Apr 14, 2019
e85aa98
Update image caption tagname from p to figcaption
mchowning Apr 15, 2019
4f81ee9
Merge remote-tracking branch 'origin/master' into issue/image_caption…
mchowning Apr 16, 2019
4c4801a
Remove `onBlurCaption` function
mchowning Apr 16, 2019
52f62bb
Merge remote-tracking branch 'origin/master' into issue/image_caption…
mchowning Apr 22, 2019
740e9fd
Merge remote-tracking branch 'origin/master' into issue/image_caption…
mchowning Apr 25, 2019
5c442a4
Merge remote-tracking branch 'origin/master' into issue/image_caption…
mchowning Apr 26, 2019
91c3662
Remove tagName from caption RichText component
mchowning Apr 26, 2019
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
23 changes: 15 additions & 8 deletions packages/block-editor/src/components/rich-text/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export class RichText extends Component {
this.onSelectionChange = this.onSelectionChange.bind( this );
this.valueToFormat = this.valueToFormat.bind( this );
this.willTrimSpaces = this.willTrimSpaces.bind( this );
this.getHtmlToRender = this.getHtmlToRender.bind( this );
this.state = {
start: 0,
end: 0,
Expand Down Expand Up @@ -643,6 +644,19 @@ export class RichText extends Component {
return false;
}

getHtmlToRender( record, tagName ) {
// Save back to HTML from React tree
const value = this.valueToFormat( record );

if ( value === undefined || value === '' ) {
this.lastEventCount = undefined; // force a refresh on the native side
return '';
} else if ( tagName ) {
return `<${ tagName }>${ value }</${ tagName }>`;
}
return value;
}

render() {
const {
tagName,
Expand All @@ -653,14 +667,7 @@ export class RichText extends Component {
} = this.props;

const record = this.getRecord();
// Save back to HTML from React tree
const value = this.valueToFormat( record );
let html = `<${ tagName }>${ value }</${ tagName }>`;
// We need to check if the value is undefined or empty, and then assign it properly otherwise the placeholder is not visible
if ( value === undefined || value === '' ) {
html = '';
this.lastEventCount = undefined; // force a refresh on the native side
}
const html = this.getHtmlToRender( record, tagName );

let minHeight = styles[ 'block-editor-rich-text' ].minHeight;
if ( style && style.minHeight ) {
Expand Down
56 changes: 44 additions & 12 deletions packages/block-library/src/image/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import React from 'react';
import { View, ImageBackground, TextInput, Text, TouchableWithoutFeedback } from 'react-native';
import { View, ImageBackground, Text, TouchableWithoutFeedback } from 'react-native';
import {
subscribeMediaUpload,
requestMediaPickFromMediaLibrary,
Expand Down Expand Up @@ -63,6 +63,7 @@ class ImageEdit extends React.Component {
progress: 0,
isUploadInProgress: false,
isUploadFailed: false,
isCaptionSelected: false,
};

this.mediaUpload = this.mediaUpload.bind( this );
Expand All @@ -74,6 +75,7 @@ class ImageEdit extends React.Component {
this.onSetLinkDestination = this.onSetLinkDestination.bind( this );
this.onImagePressed = this.onImagePressed.bind( this );
this.onClearSettings = this.onClearSettings.bind( this );
this.onFocusCaption = this.onFocusCaption.bind( this );
}

componentDidMount() {
Expand Down Expand Up @@ -101,6 +103,14 @@ class ImageEdit extends React.Component {
this.removeMediaUploadListener();
}

componentWillReceiveProps( nextProps ) {
// Avoid a UI flicker in the toolbar by insuring that isCaptionSelected
// is updated immediately any time the isSelected prop becomes false
this.setState( ( state ) => ( {
isCaptionSelected: nextProps.isSelected && state.isCaptionSelected,
} ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am guarding the isCaptionSelected state here with the image block's isSelected prop in order to avoid a noticeable flicker in the toolbar buttons when changing to another block with buttons. There seems to be a lag between the image block's isSelected prop being updated to false and its isCaptionSelected state being updated to false by the onCaptionBlur function (which is passed as the caption's onBlur prop). That lag created a split second where the toolbar buttons for both the caption and the newly-selected block were shown at the same time.

My original approach was to just set the caption's isSelected prop in the image block's render function as this.props.isSelected && this.state.isCaptionSelected. But this did not work when:

(1) the caption was selected (so the image's isCaptionSelected state was true);
(2) the caption had no text; and
(3) another block was selected, causing the image's isSelected prop to change to false.

When the new block is selected in (3), the caption's onBlur prop function (the onCaptionBlur function) would not be called and update the image block's isCaptionSelected state to be false becasuse an empty caption is not even rendered once the image block's isSelected prop is false:

gutenberg/packages/block-library/src/image/edit.native.js

Lines 420 to 422 in a738017

{ ( ! RichText.isEmpty( caption ) > 0 || isSelected ) && (
<View style={ { padding: 12, flex: 1 } }>
<RichText

Because the isCaptionSelected state is never updated to false, if the user re-selects the image with the empty caption, the empty caption would appear and immediately be selected even though the user did not tap on the caption (the user couldn't have directly tapped on the caption because empty captions are not shown until the related image is selected). If the caption was not empty, then this bug does not appear because the caption would always render, allowing its onBlur prop function to update the isCaptionSelected state to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!
Maybe for this one it would be worth to add a short comment in code?
So someone else (or future us) that sees it will understand the intention easily.

Copy link
Contributor Author

@mchowning mchowning Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea. I included this info in the commit body, but it makes sense to add a brief explanation here in the code.

I pushed an update addressing this.

}

onImagePressed() {
const { attributes } = this.props;

Expand All @@ -109,6 +119,11 @@ class ImageEdit extends React.Component {
} else if ( attributes.id && ! isURL( attributes.url ) ) {
requestImageFailedRetryDialog( attributes.id );
}

this._caption.blur();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this so that when an image's caption is selected and the user taps on the image, the caption will lose focus and the toolbar will be updated to only contain the edit image button.

this.setState( {
isCaptionSelected: false,
} );
}

mediaUpload( payload ) {
Expand Down Expand Up @@ -210,6 +225,17 @@ class ImageEdit extends React.Component {
];
}

onFocusCaption() {
if ( this.props.onFocus ) {
this.props.onFocus();
}
Copy link
Contributor Author

@mchowning mchowning Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This this.props.onFocus() call avoids two issues:

  1. Going from another RichText component to an image caption would result in the toolbar having the buttons for both blocks (testing scenario 8); and
  2. None of the styling toolbar buttons would appear when switching from an unsupported block to a non-empty image caption (testing scenario 9).

Adding this same logic to the onBlurCaption to call this.props.onBlur did not appear to be strictly necessary because this.props.onBlur was always undefined in my testing, but I thought it was better to add it so that onBlurCaption and onFocusCaption would have consistent behavior.

if ( ! this.state.isCaptionSelected ) {
this.setState( {
isCaptionSelected: true,
} );
}
}

render() {
const { attributes, isSelected, setAttributes } = this.props;
const { url, caption, height, width, alt, href } = attributes;
Expand Down Expand Up @@ -338,9 +364,11 @@ class ImageEdit extends React.Component {
>
<View style={ { flex: 1 } }>
{ showSpinner && <Spinner progress={ progress } /> }
<BlockControls>
{ toolbarEditButton }
</BlockControls>
{ ( ! this.state.isCaptionSelected ) &&
<BlockControls>
{ toolbarEditButton }
</BlockControls>
}
<InspectorControls>
<ToolbarButton
title={ __( 'Image Settings' ) }
Expand Down Expand Up @@ -392,19 +420,23 @@ class ImageEdit extends React.Component {
} }
</ImageSize>
{ ( ! RichText.isEmpty( caption ) > 0 || isSelected ) && (
<View
style={ { padding: 12, flex: 1 } }
<View style={ { padding: 12, flex: 1 } }
accessible={ true }
accessibilityLabel={ __( 'Image caption' ) + __( '.' ) + ' ' + ( isEmpty( caption ) ? __( 'Empty' ) : caption ) }
accessibilityRole={ 'button' }
>
<TextInput
style={ { textAlign: 'center' } }
fontFamily={ this.props.fontFamily || ( styles[ 'caption-text' ].fontFamily ) }
underlineColorAndroid="transparent"
value={ caption }
<RichText
setRef={ ( ref ) => {
this._caption = ref;
} }
placeholder={ __( 'Write caption…' ) }
onChangeText={ ( newCaption ) => setAttributes( { caption: newCaption } ) }
value={ caption }
onChange={ ( newCaption ) => setAttributes( { caption: newCaption } ) }
onFocus={ this.onFocusCaption }
onBlur={ this.props.onBlur } // always assign onBlur as props
isSelected={ this.state.isCaptionSelected }
fontSize={ 14 }
underlineColorAndroid="transparent"
/>
</View>
) }
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/image/styles.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@
align-items: center;
}

.caption-text {
font-family: $default-regular-font;
}

.clearSettingsButton {
color: $alert-red;
}