Skip to content

Commit

Permalink
Add Media File and Attachment Page options to the native Image block …
Browse files Browse the repository at this point in the history 
…Link To menu (#34846)

* Add initial UI for Image Link To options

Displays "Media" and "Attachment Page" options within the Link To
settings for the Image block. The logic does not work for the new
options, and the implementation is rudimentary. Several `TODO(David)`
are in place for remaining work.

* Intercept link settings navigation with callback instead of string

Using a callback avoids placing context-specific props reference inside
of link settings. I.e. `imageUrl` and `attachmentPageUrl` are no longer
referenced within link settings.

* Align labels with web UI

Reusing label text from the web will likely increase familiarity and
reduce confusion for users.

* Display selected link to option in Image block settings

Display the currently selected link to option in the top-level Image
block settings.

* Reset link destination to None when removing Custom URL

* Indicate selected link to option with check mark icon

* Fix comment typo

* Display correct value mask when URL value is set

Relying upon the `placeholder` did not work correctly whenever a `value`
was set, i.e. in the case of a Custom URL. Additionally, overloading the
`placeholder` prop is confusing. The new `valueMask` prop hopefully
better communicates the intent.

* Reference Cell attached to BottomSheet component

Avoid superfluous import.

* Fix incorrect prop references

The prior references are non-existent.

* Correctly mask Custom URL value when a URL is set via a different option

Selecting Media File or Attachment Page sets a URL value. Originally,
this caused the Custom URL option to display the URL value, rather than
persistently displaying its "Search or type URL" placeholder. The Custom
URL option should only display an actual URL value if it is manually
typed or selected within the link picker.

* Avoid passing current URL to link picker if it is not custom

When the Media File or Attachment Page options are selected, a URL value
is set. Previously, this value was passed to the link picker when the
Custom URL option was subsequently selected. This meant the link picker
would be pre-populated with a (likely unidentifiable) URL that was not
manually entered by the user, which could be confusing.

* Fix stale URL value

Referencing `inputValue` from within LinkSettingsScreen resulted in a
stale value to from `useRoute`. By referencing, the `url` where the
callback is defined, we avoid this issue.

* Fix unnecessary updates to attributes that resulted in bug

Set more appropriate defaults for link settings, the new values match
the sibling state values of the components.  Previously the `undefined`
values for `label` and `rel` differed from the empty string values in
component state. This change helps avoid unnecessary calls to
`setAttributes` that occurred when the BottomSheet was closed.

In addition to being unnecessary, these additional `setAttributes` calls
caused the Image edit component to erroneously change the
`linkDestination` even though the URL had not changed.

* Fix attachment page URL

* Fix incorrect link destination set when toggling new tab or setting rel

When "Open in new tab" or "Link Rel" is modified, the URL is again
passed to be set within `setMappedAttributes`. This caused the
`linkDestination` to improperly be modified. This change now avoid
settings `linkDestination` if the URL is not changing.

* Conditionally display Attachment Page option if URL exists

External images will not have an attachment page URL, so this option
should not be displayed when the URL is undefined.

* Fix alignment for left-aligned selected icon

Toggling the icon opacity rather than the presence of the icon itself
produces the desired alignment for both selected an unselected options.

* Rename style selector to better describe intent

* Remove irrelevant comment regarding setAttributes

After reviewing the logic within `LinkSettings`, it largely manages
invoking `setAttributes` in different scenarios. Therefore, using
`setAttributes` directly makes sense for this component.

* Rename ImageOptionsScreen to ImageLinkDestinationsScreen

Attempt to better describe component intent.

* Add LinkDestination abstraction

Avoid repetition to reduce likelihood of bugs.

* Fix stale link picker value and React Navigation warning

Passing `setAttributes` resulted in a stale value within the link picker
that prohibited setting the same Custom URL twice if attempted without
closing the bottom sheet entirely. E.g. None => Custom URL => None =>
Custom URL.

Additionally, React Navigation warns about passing non-serializable
values as route params, e.g. functions.

These changes addresses both issues.

* Rename image link destination screen for consistency

* Remove Custom URL placeholder

The design of the link destination picker has less width available,
which caused layout issues for the placeholder, particularly on Android
where spacing is larger.

* Reduce spacing around left-side cell icons on Android

This diverges from Material Design guides, but provides the additional
room we require for the new image link destination picker. The Custom
URL option needs to display both an selected icon (checkmark) on the
left side as well as a value and right chevron on the right side.

* Revert "Reduce spacing around left-side cell icons on Android"

This reverts commit 2ef5f65.

* Fix select icon color

* Fix selected icon styles for dark mode

Utilize colors as directed by designer.

* Update selected icon colors

Change to colors requested by designer.

* Update change log

* Simplify unnecessarily complex conditional

`iconStyle` has a default value of `{}`.

* Reference screen name cache rather than string literal

Attempt to avoid typos.

* Remove unnecessary mappedAttributes assignment

Now that we pass each prop individually to the child component, it is
arguably simpler to destructure the props required.

* Reuse linkDestination parameter instead of new local assignment

Reassigning a destructured function parameter, instead of creating a new
local assignment, allows for fewer conditionals and less code.

* Refactor selected status of LinkDestination

Relocate the logic to allow for a single `isSelected` prop to improve
readability.

* Remove unused code

There are no references to this method in the source.

* Remove undefined callback

This prop referenced an undefined class method.

* Replace style-based key with index key

Stringifying the styles appears unnecessary, and it was also causing
missing key warnings in tests as the styles are undefined in that
context. Leveraging the index appears to be enough to satisfy both the
app and the tests.

* Refactor existing Image edit tests to avoid referencing internals

Test from the "public" API of the component and its output, rather than
internal methods. I.e. press rendered buttons instead of calling methods
by name.

* Remove unnecessary native file extension reference

Metro resolves native files by default.

* Avoid global act by awaiting specific change within Image component

The Image component invokes `getMedia` within its selector. This results
in an async resolver running, which leads to changes to the component
after the async work completes. Rather than wrapping the entire test
helper with `act`, we should target the specific change within the test
file.

* Add missing React Navigation testing setup

Per React Navigation documentation, this file should be included.
https://bit.ly/3n6Rotv

* Add note regarding React Navigation async test error

There appears to be a bug in React Navigation that causes error logs in
Jest tests. We must await the `fireEvent` that triggers the navigation
event in order to suppress the error. https://git.io/Ju35Z

* Add Image Link To tests

* Fix incorrect Link To value mask caused by stale route parameters

Originally, ImageLinkDestinationsScreen relied upon (1) differing route
parameter names from LinkPicker and (2) explicitly set `linkDestination`
while LinkPicker did not. Stale route parameters resulted in erroneous
attribute updates when swapping the link destination from Media File >
Custom URL > Media File.

This change addresses the stale route parameters by (1) unifying the
route parameter names between the two components and (2) unifying the
approach for setting `linkDestination` to be computed based upon the
URL. The URL, represented as the `inputValue` route parameter, is now
the sole route parameter.

* Fix erroneous URL display within Custom URL input

The image URL may contain query parameters for aspects like display
dimensions. This caused a comparison based upon the URL to unexpectedly
mismatch. By leveraging `linkDestination` instead, we correctly hide the
URL from the Custom URL input whenever Media File or Attachment Page is
selected.

* Format initialHtml seed consistently
  • Loading branch information
@dcalhoun
dcalhoun committed Nov 3, 2021
1 parent 3e84b56 commit f871c50
Show file tree
Hide file tree
Showing 16 changed files with 553 additions and 116 deletions.
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-list/index.native.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { __ } from '@wordpress/i18n';
*/
import styles from './style.scss';
import BlockListAppender from '../block-list-appender';
import BlockListItem from './block-list-item.native';
import BlockListItem from './block-list-item';
import { store as blockEditorStore } from '../../store';

const BlockListContext = createContext();
Expand Down
7 changes: 7 additions & 0 deletions packages/block-editor/src/components/block-settings/container.native.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
BottomSheet,
ColorSettings,
FocalPointSettingsPanel,
ImageLinkDestinationsScreen,
LinkPickerScreen,
} from '@wordpress/components';
import { compose } from '@wordpress/compose';
Expand All @@ -21,6 +22,7 @@ export const blockSettingsScreens = {
color: 'Color',
focalPoint: 'FocalPoint',
linkPicker: 'linkPicker',
imageLinkDestinations: 'imageLinkDestinations',
};

function BottomSheetSettings( {
Expand Down Expand Up @@ -75,6 +77,11 @@ function BottomSheetSettings( {
returnScreenName={ blockSettingsScreens.settings }
/>
</BottomSheet.NavigationScreen>
<BottomSheet.NavigationScreen
name={ blockSettingsScreens.imageLinkDestinations }
>
<ImageLinkDestinationsScreen { ...props } />
</BottomSheet.NavigationScreen>
</BottomSheet.NavigationContainer>
</BottomSheet>
);
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-styles/preview.native.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ function StylePreview( { onPress, isActive, style, url } ) {
);

const getOutline = ( outlineStyles ) =>
outlineStyles.map( ( outlineStyle ) => {
outlineStyles.map( ( outlineStyle, index ) => {
return (
<Animated.View
style={ [ outlineStyle, { opacity }, styles[ name ] ] }
key={ JSON.stringify( outlineStyle ) }
key={ index }
/>
);
} );
Expand Down
176 changes: 114 additions & 62 deletions packages/block-library/src/image/edit.native.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@
* External dependencies
*/
import { View, TouchableWithoutFeedback } from 'react-native';
import { useRoute } from '@react-navigation/native';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Component, useEffect } from '@wordpress/element';
import {
requestMediaImport,
mediaUploadSync,
Expand Down Expand Up @@ -41,6 +42,7 @@ import {
BlockAlignmentToolbar,
BlockStyles,
store as blockEditorStore,
blockSettingsScreens,
} from '@wordpress/block-editor';
import { __, _x, sprintf } from '@wordpress/i18n';
import { getProtocol, hasQueryArg } from '@wordpress/url';
Expand All @@ -63,6 +65,7 @@ import styles from './styles.scss';
import { getUpdatedLinkTargetSettings } from './utils';

import {
LINK_DESTINATION_NONE,
LINK_DESTINATION_CUSTOM,
LINK_DESTINATION_ATTACHMENT,
LINK_DESTINATION_MEDIA,
Expand All @@ -76,6 +79,101 @@ const getUrlForSlug = ( image, sizeSlug ) => {
return image?.media_details?.sizes?.[ sizeSlug ]?.source_url;
};

function LinkSettings( {
attributes,
image,
isLinkSheetVisible,
setMappedAttributes,
} ) {
const route = useRoute();
const { href: url, label, linkDestination, linkTarget, rel } = attributes;

// Persist attributes passed from child screen
useEffect( () => {
const { inputValue: newUrl } = route.params || {};

let newLinkDestination;
switch ( newUrl ) {
case attributes.url:
newLinkDestination = LINK_DESTINATION_MEDIA;
break;
case image?.link:
newLinkDestination = LINK_DESTINATION_ATTACHMENT;
break;
case '':
newLinkDestination = LINK_DESTINATION_NONE;
break;
default:
newLinkDestination = LINK_DESTINATION_CUSTOM;
break;
}

setMappedAttributes( {
url: newUrl,
linkDestination: newLinkDestination,
} );
}, [ route.params?.inputValue ] );

let valueMask;
switch ( linkDestination ) {
case LINK_DESTINATION_MEDIA:
valueMask = __( 'Media File' );
break;
case LINK_DESTINATION_ATTACHMENT:
valueMask = __( 'Attachment Page' );
break;
case LINK_DESTINATION_CUSTOM:
valueMask = __( 'Custom URL' );
break;
default:
valueMask = __( 'None' );
break;
}

const linkSettingsOptions = {
url: {
valueMask,
autoFocus: false,
autoFill: true,
},
openInNewTab: {
label: __( 'Open in new tab' ),
},
linkRel: {
label: __( 'Link Rel' ),
placeholder: _x( 'None', 'Link rel attribute value placeholder' ),
},
};

return (
<PanelBody title={ __( 'Link Settings' ) }>
<LinkSettingsNavigation
isVisible={ isLinkSheetVisible }
url={ url }
rel={ rel }
label={ label }
linkTarget={ linkTarget }
setAttributes={ setMappedAttributes }
withBottomSheet={ false }
hasPicker
options={ linkSettingsOptions }
showIcon={ false }
onLinkCellPressed={ ( { navigation } ) => {
navigation.navigate(
blockSettingsScreens.imageLinkDestinations,
{
inputValue: attributes.href,
linkDestination: attributes.linkDestination,
imageUrl: attributes.url,
attachmentPageUrl: image?.link,
}
);
} }
/>
</PanelBody>
);
}

export class ImageEdit extends Component {
constructor( props ) {
super( props );
Expand All @@ -96,7 +194,6 @@ export class ImageEdit extends Component {
);
this.updateMediaProgress = this.updateMediaProgress.bind( this );
this.updateImageURL = this.updateImageURL.bind( this );
this.onSetLinkDestination = this.onSetLinkDestination.bind( this );
this.onSetNewTab = this.onSetNewTab.bind( this );
this.onSetSizeSlug = this.onSetSizeSlug.bind( this );
this.onImagePressed = this.onImagePressed.bind( this );
Expand All @@ -108,25 +205,6 @@ export class ImageEdit extends Component {
);
this.setMappedAttributes = this.setMappedAttributes.bind( this );
this.onSizeChangeValue = this.onSizeChangeValue.bind( this );

this.linkSettingsOptions = {
url: {
label: __( 'Image Link URL' ),
placeholder: __( 'Add URL' ),
autoFocus: false,
autoFill: true,
},
openInNewTab: {
label: __( 'Open in new tab' ),
},
linkRel: {
label: __( 'Link Rel' ),
placeholder: _x(
'None',
'Link rel attribute value placeholder'
),
},
};
}

componentDidMount() {
Expand Down Expand Up @@ -282,13 +360,6 @@ export class ImageEdit extends Component {
} );
}

onSetLinkDestination( href ) {
this.props.setAttributes( {
linkDestination: LINK_DESTINATION_CUSTOM,
href,
} );
}

onSetNewTab( value ) {
const updatedLinkTarget = getUpdatedLinkTargetSettings(
value,
Expand Down Expand Up @@ -383,45 +454,23 @@ export class ImageEdit extends Component {
: width;
}

setMappedAttributes( { url: href, ...restAttributes } ) {
setMappedAttributes( { url: href, linkDestination, ...restAttributes } ) {
const { setAttributes } = this.props;
if ( ! href && ! linkDestination ) {
linkDestination = LINK_DESTINATION_NONE;
} else if ( ! linkDestination ) {
linkDestination = LINK_DESTINATION_CUSTOM;
}

return href === undefined
? setAttributes( {
...restAttributes,
linkDestination: LINK_DESTINATION_CUSTOM,
} )
return href === undefined || href === this.props.attributes.href
? setAttributes( restAttributes )
: setAttributes( {
...restAttributes,
linkDestination,
href,
linkDestination: LINK_DESTINATION_CUSTOM,
} );
}

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

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

getAltTextSettings() {
const {
attributes: { alt },
Expand Down Expand Up @@ -583,9 +632,12 @@ export class ImageEdit extends Component {
) }
{ this.getAltTextSettings() }
</PanelBody>
<PanelBody title={ __( 'Link Settings' ) }>
{ this.getLinkSettings( true ) }
</PanelBody>
<LinkSettings
attributes={ this.props.attributes }
image={ this.props.image }
isLinkSheetVisible={ this.state.isLinkSheetVisible }
setMappedAttributes={ this.setMappedAttributes }
/>
<PanelBody
title={ __( 'Featured Image' ) }
titleStyle={ styles.featuredImagePanelTitle }
Expand Down
Loading

0 comments on commit f871c50

Please sign in to comment.