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

[Enhancement] Introduce Link Target in Image Block #9520

Merged
merged 1 commit into from Oct 24, 2018

Conversation

Projects
None yet
@nfmohit-wpmudev
Contributor

nfmohit-wpmudev commented Aug 31, 2018

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:

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

Screenshots

pull-9458-min
(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

This comment has been minimized.

ayersc3 commented Sep 13, 2018

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-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Sep 14, 2018

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:
pull-9520

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-e

This comment has been minimized.

Contributor

cristian-e commented Sep 16, 2018

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

This comment has been minimized.

ayersc3 commented Sep 17, 2018

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!

screen shot 2018-09-17 at 9 39 45 am

@nfmohit-wpmudev nfmohit-wpmudev force-pushed the nfmohit-wpmudev:update/image-block/9458 branch from e9244c0 to 42985d4 Sep 19, 2018

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Sep 19, 2018

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

This comment has been minimized.

Contributor

chrisvanpatten commented Oct 16, 2018

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.

@@ -65,6 +65,9 @@ const blockAttributes = {
type: 'string',
default: 'none',
},
linkTarget: {
type: 'boolean',

This comment has been minimized.

@talldan

talldan Oct 17, 2018

Contributor

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.

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev Oct 18, 2018

Contributor

Implemented in this commit. Thank you ❤️

@@ -219,7 +222,7 @@ export const settings = {
const figure = (
<Fragment>
{ href ? <a href={ href }>{ image }</a> : image }
{ href ? <a href={ href } target={ linkTarget ? '_blank' : null } rel={ linkTarget ? 'noreferrer noopener' : null }>{ image }</a> : image }

This comment has been minimized.

@talldan

talldan Oct 17, 2018

Contributor

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:
facebook/react#1448

This comment has been minimized.

@talldan

talldan Oct 17, 2018

Contributor

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

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev Oct 18, 2018

Contributor

Implemented in this commit. Thank you ❤️

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Oct 17, 2018

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

disabled={ isLinkURLInputDisabled }
/>
<ToggleControl
label={ __( 'Open in New Window' ) }

This comment has been minimized.

@afercia

afercia Oct 17, 2018

Contributor

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".

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev Oct 18, 2018

Contributor

Implemented in this commit. Thank you ❤️

@talldan

This comment has been minimized.

Contributor

talldan commented Oct 18, 2018

@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:

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

  1. Add an image block and upload an image (or select one from the media library).
  2. Choose an option under 'Link to' and set the link to open in a new tab.
  3. From the block menu, switch to Edit as HTML
  4. Copy the HTML and paste it into a new paragraph
  5. The pasted content should be transformed into a new image block.
  6. 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

This comment has been minimized.

Contributor

talldan commented Oct 18, 2018

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. 🤔

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

@nfmohit-wpmudev nfmohit-wpmudev force-pushed the nfmohit-wpmudev:update/image-block/9458 branch from b339d69 to b2770d0 Oct 18, 2018

@nfmohit-wpmudev

This comment has been minimized.

Contributor

nfmohit-wpmudev commented Oct 18, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Oct 19, 2018

I'd appreciate a design validation as well :)

@@ -83,7 +89,7 @@ const schema = {
children: {
...imageSchema,
a: {
attributes: [ 'href' ],
attributes: [ 'href', 'target' ],

This comment has been minimized.

@aduth

aduth Oct 19, 2018

Member

Does rel need to be here?

This comment has been minimized.

@nfmohit-wpmudev

nfmohit-wpmudev Oct 19, 2018

Contributor

@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?

This comment has been minimized.

@talldan

talldan Oct 23, 2018

Contributor

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.

This comment has been minimized.

@aduth

aduth Oct 23, 2018

Member

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.

This comment has been minimized.

@talldan

talldan Oct 24, 2018

Contributor

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.

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

2 checks passed

codecov/project 49.4% (-0.01%) compared to cbf425b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

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