[Enhancement] Introduce Link Target in Image Block by nfmohit · Pull Request #9520 · WordPress/gutenberg

nfmohit · 2018-08-31T18:12:09Z

Description

This PR closes #9458 which requests the addition of an option to open the image in a new tab under "Link Options" in the Image block.

How has this been tested?

This PR has been tested by going through the following steps:

Started a new post using the Gutenberg editor.
Added the "Image" block.
Made sure the "Open in New Window" option shows up under "Link Options".
Made sure the link target functionality works in the front-end.

Screenshots

(Apologies about the crappy screencast, had to shrink the browser down and compress for Github to accept it).

Types of changes

This PR introduces a new boolean attribute named linkTarget, which if true, applies the _blank target attribute. It adds ToggleControl in the InspectorControls, which toggles the attribute onChange.

Checklist:

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

ayersc3 · 2018-09-13T18:27:30Z

I've seen this option for opening links in new windows for some, but not all. I've noticed it available when linking the image to the media file. But, not available when linking to a custom URL. ...Thoughts?

nfmohit · 2018-09-14T08:22:01Z

Hey @ayersc3 !

Just gave this PR another quick test and the option is showing up for me for custom URLs too. Here's a screencast:

If you're testing it, could you let me know some details about your environment e.g. browser, OS, console errors (if any) and such?

Thank you ❤️

cristian-raiber · 2018-09-16T08:41:11Z

Tested the code and works fine. Just a suggestion: you can replace the onChange attribute of ToggleControl to:

onChange={ () => setAttributes( {  linkTarget: ! linkTarget } ) }

This way you no longer need the toggleAttribute function.

ayersc3 · 2018-09-17T13:40:39Z

Sorry for the delayed response, and thanks for your responses!

@nfmohit-wpmudev :
WP v4.9.8
Browser: Google Chrome (v68.0.3440.106)
OS: Mac (v10.13.6)
Guttenberg: v3.7 (As I'm writing this, I upgraded to the v3.8, but still was missing the option to "open in new window")

@Cristian-E
Thanks for the code suggestion. I'll try that today!

nfmohit · 2018-09-19T14:53:19Z

Thank you so much for the tip @Cristian-E ❤️ I've included it in the latest edited commit.

About the issue you are seeing @ayersc3, I'm extremely sorry but I wasn't able to replicate it by testing it in different environments. Just to confirm, may I know how you are testing it? Are you fetching the PR branch or just copying the code? May I know if you are re-building the npm project after the PR is merged locally? Thank you for your help ❤️

chrisvanpatten · 2018-10-16T13:29:02Z

Could we get a design review on this and potentially land it in 4.1? This and #10128 feel like easy wins for users.

talldan

Thanks for another awesome contribution @nfmohit-wpmudev 🔥🔥🔥

I've left a couple of review comments.

talldan · 2018-10-17T08:45:28Z

I wonder if we could source this attribute directly from the markup. The advantage is that the setting is correctly applied if the user pastes some html containing an image wrapped in a link into Gutenberg. It'd looks something like this:

linkTarget: { type: 'string', source: 'attribute', selector: 'figure > a', attribute: 'target', }

the rendered element would become:

{ href ? <a href={ href } target={ linkTarget } rel={ linkTarget === '_blank' ? 'noreferrer noopener' : undefined }>{ image }</a> : image }

and the value we pass into the toggle control would have to be calculated.

Implemented in this commit. Thank you ❤️

talldan · 2018-10-17T08:51:13Z

I think we should make the falsey part of the ternary undefined instead of null. I believe there are some cases where null attributes in React get converted to an empty string, which would result in a property being unnecessarily added to the element:
react/react#1448

Also, would be great to break this lengthy line up 😄

Implemented in this commit. Thank you ❤️

nfmohit · 2018-10-17T15:37:34Z

Implemented. Thank you so much for the review @talldan ❤️ Couldn't make it without your help in DM regarding the toggle value calculation logic ❤️

afercia · 2018-10-17T17:41:36Z

Worth considering in core all occurrences of (opens in a new window) have been changed to (opens in a new tab), see https://core.trac.wordpress.org/ticket/43803 which will be released in 5.1 (not 5.0). Even if that string is meant for visually hidden accessible text, for consistency I'd suggest to refer to "tab" and not "window".

Implemented in this commit. Thank you ❤️

talldan · 2018-10-18T10:42:18Z

@nfmohit-wpmudev This is looking really close. I just spotted one thing that I missed earlier, 'target' will need to be added to this array:
https://github.com/WordPress/gutenberg/blob/aead14ac3fd919a2fdeb510b3d492d368e506f2d/packages/block-library/src/image/index.js#L89

That ensures the attribute is parsed and set correctly. To test it you can try the following steps:

Add an image block and upload an image (or select one from the media library).
Choose an option under 'Link to' and set the link to open in a new tab.
From the block menu, switch to Edit as HTML
Copy the HTML and paste it into a new paragraph
The pasted content should be transformed into a new image block.
Check the sidebar 'Opens in a new tab' setting, it should be correctly enabled to reflect that the pasted html had the target="_blank" attribute.

Without the 'target' value in the array, you'll see that 'Opens In a New Tab' remains off, and when inspecting the HTML, target is stripped off.

talldan · 2018-10-18T10:50:23Z

It might also be good to rebase this against master - there was at least one conflict when I tested (pretty easy to resolve though). Interesting though that it's not showing in the github UI as a conflict. 🤔

nfmohit · 2018-10-18T18:42:04Z

Thank you so much for pointing out the array @talldan ❤️ It was completely out of my consideration. I've addressed that and the conflicts with master in the latest edited commit.

talldan

This is looking good to me now. Thanks for addressing all the changes so quickly, @nfmohit-wpmudev ❤️

We should hold off merging until the 4.1 milestone has been shipped, but once that's done this is good to go.

youknowriad · 2018-10-19T10:07:32Z

I'd appreciate a design validation as well :)

aduth · 2018-10-19T12:31:14Z

 			...imageSchema,
 			a: {
-				attributes: [ 'href' ],
+				attributes: [ 'href', 'target' ],


Does rel need to be here?

@aduth I'm honestly not completely sure but according to this comment, it seems the rel attribute gets set correctly when the HTML is copied over to a new paragraph element even without rel attribute added to the array. @talldan Any comments here?

At the moment rel is ignored by the parser, butrel="noreferrer noopener" is added to any link with target="_blank" by the save function. I think that's a sensible default. I suppose some users might want a finer level of control over the attribute.

I think this is probably ok as a start, the code doesn't preclude more handling of rel being added later.

It's not clear to me as someone implementing a block with a schema why the parser would disregard rel. I'm left confused why I should or shouldn't include the attribute here.

It doesn't seem to be something mentioned in the handbook (nothing about raw transformations in general, as far as I could see), so potentially a gap in the information we provide.

karmatosed

I am approving as can see it being useful but have to say that sidebar is getting really packed with options for the image block. I would encouraging future thinking about how we can reduce the options and iterate.

ajitbohra added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Aug 31, 2018

therajumandapati mentioned this pull request Sep 20, 2018

Enable opening links in new tab for Button block #10073

Closed

chrisvanpatten mentioned this pull request Oct 5, 2018

Insert Link to image needs "open in new tab" option #6891

Closed

mtias added the [Feature] Media Anything that impacts the experience of managing media label Oct 7, 2018

chrisvanpatten requested review from a team and karmatosed October 16, 2018 13:27

talldan reviewed Oct 17, 2018

View reviewed changes

afercia reviewed Oct 17, 2018

View reviewed changes

afercia mentioned this pull request Oct 17, 2018

Change all the occurrences of new window to new tab #10697

Closed

gziolo added this to the 4.2 - API freeze milestone Oct 18, 2018

Introduce link target in image block

b2770d0

talldan approved these changes Oct 19, 2018

View reviewed changes

aduth reviewed Oct 19, 2018

View reviewed changes

karmatosed approved these changes Oct 19, 2018

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 23, 2018

talldan merged commit d4e097c into WordPress:master Oct 24, 2018

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Introduce link target in image block (WordPress#9520)

402029d

getsource mentioned this pull request Oct 31, 2018

Consider feature parity for "Image Details" TinyMCE functionality #7504

Closed

4 tasks

Uh oh!

Conversation

nfmohit commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

ayersc3 commented Sep 13, 2018

Uh oh!

nfmohit commented Sep 14, 2018

Uh oh!

cristian-raiber commented Sep 16, 2018

Uh oh!

ayersc3 commented Sep 17, 2018

Uh oh!

nfmohit commented Sep 19, 2018

Uh oh!

chrisvanpatten commented Oct 16, 2018

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfmohit Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfmohit Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfmohit commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talldan commented Oct 18, 2018

Uh oh!

nfmohit commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfmohit Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talldan Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karmatosed left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

nfmohit commented Aug 31, 2018 •

edited

Loading

nfmohit Oct 18, 2018 •

edited

Loading

nfmohit Oct 18, 2018 •

edited

Loading

talldan commented Oct 18, 2018 •

edited

Loading

nfmohit commented Oct 18, 2018 •

edited

Loading

nfmohit Oct 19, 2018 •

edited

Loading

talldan Oct 23, 2018 •

edited

Loading