Try to figure out which is the right LinkDestination value for an image

[jesusangel]

I want to always link the images in my posts to its source url. This way, users can click on the thumbnail and see the image full size. In Gutenberg's image block, the default setting is not to link the images, so I have to manually change the LinkDestination select for each image I insert in the posts.

Users can change the default value of LinkDestination with a plugin and some JavaScript like this:

       function modifyImageLinkDestinationDefault( settings, name ) {

           // Override core default value for LinkDestination and change it to media 
           if ( name == "core/image" ) {
                // Set image block default destination.
                settings.attributes.linkDestination.default = "media";
            }

            return settings;
        }

        // Add settings filter to block registration.
        wp.hooks.addFilter(
            "blocks.registerBlockType",
            "my-plugin/modify-image-link-destination-default",
            modifyImageLinkDestinationDefault
        );

The actual code of the Gutenberg's image block doesn't work well when it finds old posts with no LinkDestination attribute, as no attribute means default value, but the new default value isn't the same as the old one.

Description

I've added code to the componentDidMount method of the image block to figure out which is the right value for the LinkDestination attribute based on the innerHTML. This work is based on @SeanDS pull request #13797 and closes issue #13749.

How has this been tested?

I used the latest development release of Gutenberg (cloned at 313974f), and WordPress 5.1, using docker images. I used the plugin showed above to change the default link target to media. I added several images with different LinkDestination values and I edited again after disabling the plugin.

This change doesn't affect other areas of the code.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• [ X] My code is tested.
• [ X] My code follows the WordPress code style.
• [ X] My code follows the accessibility standards.
• [X ] My code has proper inline documentation.
• [ X] I've included developer documentation if appropriate.

[jesusangel]

I'm sorry. I misread the output from the npm run lint command and there were some errors. I've fixed them so now I hope the code will pass the checks.

[SeanDS]

@jesusangel, thanks a lot for taking up this bug fix! It's not clear from your description whether you also managed to fix what @talldan flagged?

I spotted an issue in testing - the Link To 'Attachment' option no longer works. Left a comment mentioning where the issue is in the code.

The changes he suggested I make didn't seem to work.

[jesusangel]

Hello @SeanDS,

I made the changes sugested by @talldan. I didn't get the this.props.image is null error. The buildLinkAttributes is called when the image is selected and after changing the LinkDestination value. In the first case, the image argument is null, but the code works fine as it uses the attributes param.

buildLinkAttributes( value, attributes, image ) {
		let href;

		if ( value === LINK_DESTINATION_NONE ) {
			href = undefined;
		} else if ( value === LINK_DESTINATION_MEDIA ) {
			href = ( image && image.source_url ) || attributes.url;
		} else if ( value === LINK_DESTINATION_ATTACHMENT ) {
			href = image && image.link;
		} else {
			href = attributes.href;
		}

		return {
			linkDestination: value,
			href,
		};
	}
`

Nevertheless, while testing the code again to test the problem that you had, I've realised that when the user inserts an image using an URL and the default LinkDestination is set to media, the URL textbox doesn't get updated with the image URL. I have to make come changes to the onSelectURL method.

[jesusangel]

Hello @SeanDS,

I didn't get the this.props.image is null error. After the image is selected and the onSelectImage event is triggered, there's no this.props.image but the code manages that by selecting another variable, for example:

href = ( image && image.source_url ) || attributes.url;

When I was testing again this issue, I realised that I haven't changed the onSelectURL event handler to manage the possible case of a default linkDestination value different than none. I'm going to make another commit to fix that.

Kind regards,

[draganescu]

Hi @jesusangel did you get to do this last change you mention?

I haven't changed the onSelectURL event handler to manage the possible case of a default linkDestination value different than none.

[jesusangel]

Hi @draganescu ,

Yes, I fixed it in commit d325b35bc on march 14th.

Kind regards,

[okainov]

Any update on this one? Would be really nice to have it in

[paaljoachim]

Hi @jesusangel

Is there anything else that is needed for this PR?
Or is it ready for a final review and merge?
Thanks..:)

[jesusangel]

Hi,

It's been a while since I submited the pull request, but I think it's ready for final review and merge.

Kind regards,

[paaljoachim]

Hey @jesusangel

I have noticed that older PR's need to be rebased. Could you please do so.
Then someone could take a closer look.

@SeanDS @talldan @jorgefilipecosta

[jesusangel]

Hey @jesusangel

I have noticed that older PR's need to be rebased. Could you please do so.
Then someone could take a closer look.

@SeanDS @talldan @jorgefilipecosta

Hi,

I've just rebased my pull request on master.

Kind regards,

[paaljoachim]

Thank you @jesusangel!

@talldan @SeanDS could you take a look?

[okainov]

I've just rebased my pull request on master.

I'm not a reviewer, but seems you did it very wrong (now your PR contains 1100+ commits and 2000+ affected files). Please rebase to keep only related commits in your MR so people can understand what's going on. Usually git fetch upstream && git rebase upstream/master should make the deal, however currently maybe it may be easier to just recreate a branch and just cherry-pick your changes on top

[swissspidy]

Usually instead of rebasing I personally just do git merge master to get the latest changes from master into my branch. Way easier than messing with history through rebasing.

[okainov]

@swissspidy very depends on both team and the person :) If one knows how to do rebases, the final history will be much cleaner than the one bloated with dozens of merges. But it's easier to screw up the stuff with rebase, it's true.

[jesusangel]

Hi @okainov,

You are right, I've screw up the stuff with the rebase :-(

I'm trying to fix it, but now the PR seems to be closed. There are no commits nor files changed.

[jesusangel]

I've created a new PR: #17401