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

Add Image Block 'Link to' setting #7679

Merged
merged 16 commits into from Jul 5, 2018

Conversation

Projects
None yet
4 participants
@talldan
Contributor

talldan commented Jul 3, 2018

Description

closes #5309

Adds a new 'Link To' setting in the image block settings, similar to the 'Link To' option in the old editor.

Removes the old url editor toolbar option for the image block (this could be kept, though would need a bit of thought about how the two could co-exist)

How has this been tested?

  • Manual testing. Tested each of the settings individually. Tested converting from a classic editor image to ensure the link url is retained in the image block.
  • Added 'full-content' tests for various settings of the new linkDestination attribute

Screenshots

link-to-update

Types of changes

Backwards compatibility with classic editor images.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan self-assigned this Jul 3, 2018

@talldan talldan changed the title from Add/image block link to setting to Add Image Block 'Link to' setting Jul 3, 2018

@@ -52,6 +51,13 @@ import ImageSize from './image-size';
*/
const MIN_SIZE = 20;
const LINK_DESTINATION_OPTIONS = {
NONE: 'NONE',

This comment has been minimized.

@talldan

talldan Jul 3, 2018

Contributor

Unsure about whether to use all caps for these constant properties. Could also make them individual consts instead of putting them in an object.

This comment has been minimized.

@noisysocks

noisysocks Jul 4, 2018

Member

Could also make them individual consts instead of putting them in an object.

I'd go with this 🙂

test( 'block edit matches snapshot', () => {
const wrapper = blockEditRender( name, settings );
expect( wrapper ).toMatchSnapshot();

This comment has been minimized.

@talldan

talldan Jul 3, 2018

Contributor

This seems to fail, though unsure why as it's the same test used on other blocks:

TypeError: Cannot read property 'getMedia' of undefined

      486 | export default compose( [
      487 | 	withSelect( ( select, props ) => {
    > 488 | 		const { getMedia } = select( 'core' );
      489 | 		const { getEditorSettings } = select( 'core/editor' );
      490 | 		const { id } = props.attributes;
      491 | 		const { maxWidth, isRTL } = getEditorSettings();

      at core-blocks/image/edit.js:488:11

This comment has been minimized.

@noisysocks

noisysocks Jul 4, 2018

Member

I think this is because blockEditRender skips the necessary bootstrapping for select() to work, namely wp.data.registerStore(). We could export a non-enhanced version of the edit component for testing. Or, we could just not bother—I don't think a snapshot test is necessarily worth the effort here 🙂

@talldan

This comment has been minimized.

Contributor

talldan commented Jul 3, 2018

@karmatosed Would be great to get some design feedback on this one if you get a moment.

In particular I'm wondering if the input field for entering a custom url could be improved.

This is what I've done:
link-to-dropdown

Here's a gif of the classic editor for contrast:
link-to-classic

@talldan talldan requested a review from karmatosed Jul 3, 2018

@karmatosed

@talldan I think if we have a disabled state on first load of the input that would make more sense. As it's a guide not probably what someone will have? Then you can click in and it changes to active input state?

@talldan

This comment has been minimized.

Contributor

talldan commented Jul 4, 2018

@karmatosed I've made some changes along those lines. Since the input field with the url in it will always be visible, I've moved the two fields into their own section in the settings so that it feels contextually better. That feels a lot tidier to me and is also consistent with the new File Block. Here's a gif, let me know what you think:

link-to-update

@karmatosed

This looks good to me now. I am not totally happy with the way the input looks but that's using the default one so not something to fix in this PR. Let's get it in! Thanks for all your hardwork on this.

@tofumatt

I think there are a few minor things to address here first… also it would be nice if we could add tests for this, but I see there aren't any existing ones so perhaps that's a rabbit hole. 😒

label={ __( 'Link URL' ) }
value={ href }
onChange={ this.onSetCustomHref }
placeholder={ ! isLinkUrlInputDisabled && 'https://' }

This comment has been minimized.

@tofumatt

tofumatt Jul 5, 2018

Member

Subtle hint toward an encrypted web, eh? 😄

@@ -113,8 +116,11 @@ export const settings = {
const align = alignMatches ? alignMatches[ 1 ] : undefined;
const idMatches = /(?:^|\s)wp-image-(\d+)(?:$|\s)/.exec( className );
const id = idMatches ? idMatches[ 1 ] : undefined;
const anchorEl = node.querySelector( 'a' );

This comment has been minimized.

@tofumatt

tofumatt Jul 5, 2018

Member

Can you rename this anchorElement instead of the abbreviation? Two-letter abbreviations are somewhat cryptic and unnecessry 😄

let href;
if ( value === LINK_DESTINATION_NONE ) {
href = '';

This comment has been minimized.

@tofumatt

tofumatt Jul 5, 2018

Member

Is there a reason we want this to be an empty string instead of null? null seems more appropriate.

Also, I'd just set the "none" value as the default here; start with let href = ''; (or let href = null; and then change it if the link destination is something else.

This comment has been minimized.

@noisysocks

noisysocks Jul 5, 2018

Member

Adding default: 'none' to the attribute definition itself feels right to me in this case.

This comment has been minimized.

@tofumatt

tofumatt Jul 5, 2018

Member

Yeah that's fine too. Just something over the way it's being done now would be good.

This comment has been minimized.

@talldan

talldan Jul 5, 2018

Contributor

I've added default: 'none' 👍

Also added some extra test fixtures to the full-content test, so that's a little bit of coverage.

talldan added some commits Jul 3, 2018

Add a new image block setting for 'Link to'
- Add a new SelectControl to the InspectorControls
- The options for this are sourced from a new function,
`getLinkDestinationOptions`
- `getLinkDestinationOptions` returns options for the mediaFile or the
attachment page
- Custom url to be added in a later commit
Store value of 'Link To' dropdown as separate attribute
- Introduce new attribute `linkDestination` for storing the selected
option of the 'Link To' block setting
- Use constants for the value of this dropdown
- Derive href based on value of dropdown
- Add a text field for entering a custom url
Revert "Add snapshot test for image block editor"
This reverts commit 4d707a3.

Reverted for now as blockEditRender doesn't seem to support select.
Always render the 'Custom URL' input field
This change makes the Custom URL field always visible, but renders it as
disabled unless the 'Custom URL' option is selected in the Link to
dropdown.
Usability changes for 'Custom Url' input field in image block settings
Make the placeholder only display when the image block is editable (not
disabled)
Move 'Link To' and 'Link URL' image block settings to their own Panel…
…Body

This makes it a bit more consistent with the File Block's Link To setting.

Have also changed the Custom URL field label to Link URL.
Handle setting of image block href when converting from classic to bl…
…ocks

The strategy chosen is to always set 'Link To' to 'Custom URL' if an anchor tag
exists in the classic markup. Trying to detect 'media file' or
'attachment page' could potentially be too error prone, this approach at least
ensures the image's anchor link is the same in the resulting post.
linkDestination=undefined when transforming a raw image block with no…
… <a> tag

For the raw transform on an image block, set the linkDestination to undefined
if the raw html does not contain an anchor tag (or has an anchor with no href).
This more accurately reflects that the default value of linkDestination is
undefined

talldan added some commits Jul 5, 2018

Change `href` to `undefined` when `linkDestination` of `none` is sele…
…cted

For this to work, have had to use `value={ url || '' }` on the TextControl
since only an empty string can clear a controlled react input.

@talldan talldan merged commit 3d96b88 into master Jul 5, 2018

2 checks passed

codecov/project 46.44% (-0.02%) compared to 7c44995
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the add/image-block-link-to-setting branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment