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] Use link picker in Image block #27292

Merged
merged 16 commits into from
Dec 21, 2020
Merged

[Mobile] Use link picker in Image block #27292

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c7cf587
Use LinkSettingsNavigation in Image block
mkevins Nov 26, 2020
763848c
Revert mapping url attribute in link settings
mkevins Nov 26, 2020
a33ccd8
Map url -> href attribute for link picker in image block
mkevins Nov 26, 2020
32361ef
Don't overwrite mapped attribute when undefined
mkevins Nov 26, 2020
13828c4
Fix lint
mkevins Nov 26, 2020
7637a1d
Merge branch 'master' into rnmobile/try/image-block-link-picker
mkevins Dec 10, 2020
cd94a5e
Conditionally render link options in link settings
mkevins Dec 10, 2020
419cef4
Move link settings panel in image block settings
mkevins Dec 10, 2020
cd662f2
Auto-hide "Open in new tab" in rich text link settings
mkevins Dec 16, 2020
2637e02
Merge branch 'master' into rnmobile/try/image-block-link-picker
mkevins Dec 16, 2020
281d46f
Merge branch 'master' into rnmobile/try/image-block-link-picker
mkevins Dec 17, 2020
99d8724
Merge branch 'master' into rnmobile/try/image-block-link-picker
mkevins Dec 17, 2020
f40b256
Revert "Update tt1 blocks references. (#27770)"
mkevins Dec 18, 2020
aaa54c0
Revert "Revert "Update tt1 blocks references. (#27770)""
mkevins Dec 18, 2020
4ea04ca
Fix assertion count in multi-entity-saving test
mkevins Dec 18, 2020
3038887
Merge branch 'master' into rnmobile/try/image-block-link-picker
mkevins Dec 20, 2020
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
77 changes: 48 additions & 29 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 @@ -20,11 +20,11 @@ import {
Icon,
PanelBody,
TextControl,
ToggleControl,
ToolbarButton,
ToolbarGroup,
Image,
WIDE_ALIGNMENTS,
LinkSettingsNavigation,
} from '@wordpress/components';
import {
BlockCaption,
Expand All @@ -43,8 +43,6 @@ import { doAction, hasAction } from '@wordpress/hooks';
import { compose, withPreferredColorScheme } from '@wordpress/compose';
import { withSelect } from '@wordpress/data';
import {
external,
link,
image as placeholderIcon,
textColor,
replace,
Expand Down Expand Up @@ -338,6 +336,49 @@ export class ImageEdit extends React.Component {
: width;
}

getLinkSettings() {
const { isLinkSheetVisible } = this.state;
const {
attributes: { href: url, ...unMappedAttributes },
setAttributes,
} = this.props;

const mappedAttributes = { ...unMappedAttributes, url };
const setMappedAttributes = ( { url: href, ...restAttributes } ) =>
href === undefined
? setAttributes( restAttributes )
: setAttributes( { ...restAttributes, href } );
Comment on lines +346 to +350
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The link settings sets the attribute named url, but Image block's url attribute is the URL of the image itself, and instead uses the attribute named href to represent the link URL.

Originally, to work around this, I used dynamic properties in LinkSettings to allow the consumer to customize the attribute name to be set.

Here, I reverted those changes to LinkSettings, and instead map the attibutes in ImageEdit. As the link picker is used in more places, it might make sense to re-evaluate this pattern, depending on the commonality of the possible attribute names (e.g. url, href). WDYT?

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 the dynamic properties approach. It seems fairly clean. The code ^ is a bit harder for me to understand. But I think this could be done as part of a larger refactor.

One thing that I noticed is that we always seem to set the options such as labels etc.

I think we could really benefit from having a default name for labels. so that we only pass in the options that are different from the default.

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 think we could really benefit from having a default name for labels. so that we only pass in the options that are different from the default.

Exactly my thoughts! I think we can refactor during the iteration for further consolidation described here. I think with the refactor we should have the opportunity to both reduce the surface area of the interface, and also make the components appropriately general (in particular, decoupled from particular attribute names and even setAttributes).


const options = {
url: {
label: __( 'Image Link URL' ),
placeholder: __( 'Add URL' ),
autoFocus: false,
autoFill: true,
},
openInNewTab: {
label: __( 'Open in new tab' ),
},
linkRel: {
label: __( 'Link Rel' ),
placeholder: __( 'None' ),
},
};

return (
<LinkSettingsNavigation
isVisible={ isLinkSheetVisible }
attributes={ mappedAttributes }
onClose={ this.dismissSheet }
setAttributes={ setMappedAttributes }
withBottomSheet={ false }
hasPicker
options={ options }
showIcon={ false }
/>
);
}

render() {
const { isCaptionSelected } = this.state;
const {
Expand All @@ -347,16 +388,7 @@ export class ImageEdit extends React.Component {
imageSizes,
clientId,
} = this.props;
const {
align,
url,
alt,
href,
id,
linkTarget,
sizeSlug,
className,
} = attributes;
const { align, url, alt, id, sizeSlug, className } = attributes;

const sizeOptions = map( imageSizes, ( { name, slug } ) => ( {
value: slug,
Expand Down Expand Up @@ -392,22 +424,6 @@ export class ImageEdit extends React.Component {
) }
</PanelBody>
<PanelBody>
<TextControl
icon={ link }
label={ __( 'Link To' ) }
value={ href || '' }
valuePlaceholder={ __( 'Add URL' ) }
onChange={ this.onSetLinkDestination }
autoCapitalize="none"
autoCorrect={ false }
keyboardType="url"
/>
<ToggleControl
icon={ external }
label={ __( 'Open in new tab' ) }
checked={ linkTarget === '_blank' }
onChange={ this.onSetNewTab }
/>
{ image && sizeOptionsValid && (
<CycleSelectControl
icon={ expand }
Expand All @@ -427,6 +443,9 @@ export class ImageEdit extends React.Component {
onChangeValue={ this.updateAlt }
/>
</PanelBody>
<PanelBody title={ __( 'Link Settings' ) }>
{ this.getLinkSettings( true ) }
</PanelBody>
</InspectorControls>
);

Expand Down
44 changes: 24 additions & 20 deletions packages/components/src/mobile/link-settings/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,26 +224,30 @@ function LinkSettings( {
onChange={ onChangeLabel }
/>
) }
{ options.openInNewTab && (
<ToggleControl
icon={ showIcon && external }
label={ options.openInNewTab.label }
checked={ linkTarget === '_blank' }
onChange={ onChangeOpenInNewTab }
/>
) }
{ options.linkRel && (
<TextControl
icon={ showIcon && LinkRelIcon }
label={ options.linkRel.label }
value={ linkRelInputValue }
valuePlaceholder={ options.linkRel.placeholder }
onChange={ onChangeLinkRel }
onSubmit={ onCloseSettingsSheet }
autoCapitalize="none"
autoCorrect={ false }
keyboardType="url"
/>
{ !! urlInputValue && (
<>
{ options.openInNewTab && (
<ToggleControl
icon={ showIcon && external }
label={ options.openInNewTab.label }
checked={ linkTarget === '_blank' }
onChange={ onChangeOpenInNewTab }
/>
) }
{ options.linkRel && (
<TextControl
icon={ showIcon && LinkRelIcon }
label={ options.linkRel.label }
value={ linkRelInputValue }
valuePlaceholder={ options.linkRel.placeholder }
onChange={ onChangeLinkRel }
onSubmit={ onCloseSettingsSheet }
autoCapitalize="none"
autoCorrect={ false }
keyboardType="url"
/>
) }
</>
) }
</>
);
Expand Down
33 changes: 20 additions & 13 deletions packages/format-library/src/link/modal-screens/link-settings-screen.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ const LinkSettingsScreen = ( {
}, [ navigation, route.params?.text, text ] );

return useMemo( () => {
const shouldShowLinkOptions = !! inputValue;

return (
<>
<BottomSheet.LinkCell
Expand All @@ -150,20 +152,25 @@ const LinkSettingsScreen = ( {
placeholder={ __( 'Add link text' ) }
onChangeValue={ setText }
onSubmit={ submit }
separatorType={ shouldShowLinkOptions ? undefined : 'none' }
/>
<BottomSheet.SwitchCell
icon={ external }
label={ __( 'Open in new tab' ) }
value={ opensInNewWindow }
onValueChange={ setOpensInNewWindows }
separatorType={ 'fullWidth' }
/>
<BottomSheet.Cell
label={ __( 'Remove link' ) }
labelStyle={ styles.clearLinkButton }
separatorType={ 'none' }
onPress={ removeLink }
/>
{ shouldShowLinkOptions && (
<>
<BottomSheet.SwitchCell
icon={ external }
label={ __( 'Open in new tab' ) }
value={ opensInNewWindow }
onValueChange={ setOpensInNewWindows }
separatorType={ 'fullWidth' }
/>
<BottomSheet.Cell
label={ __( 'Remove link' ) }
labelStyle={ styles.clearLinkButton }
separatorType={ 'none' }
onPress={ removeLink }
/>
</>
) }
</>
);
}, [ inputValue, text, opensInNewWindow, listProps.safeAreaBottomInset ] );
Expand Down