Add link rel and link class to image block inspector (see #7504) #7771

greatislander · 2018-07-07T13:45:40Z

Description

This is some initial work towards addressing #7504. If there's interest from maintainers, I can proceed to add the 'Open link in a new tab' checkbox and investigate automated testing of these features.

How has this been tested?

Added link rel and link class attributes via the block inspector, saved, viewed saved post to ensure that attributes were added.
Added tests to fixtures.

Screenshots

Types of changes

New feature: adds image link rel and class attributes to core image block.

Checklist:

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

karmatosed

I would like to know if we need these all, it feels like a lot to add for image. For example do we need custom CSS when we have that on the block?

greatislander · 2018-07-13T18:51:32Z

The distinction between the class on an image and the class on a link wrapping the image could certainly make a difference. I think the related issue makes a good point about maintaining existing functionality.

karmatosed · 2018-07-17T11:34:01Z

As explored in #7504 I am after considering not totally sure this is a secondary settings, this should be in the actual media library itself:

jasmussen · 2018-10-11T10:22:28Z

Good thoughts here. Rel is cool, especially because of the .

The media library interface is a little old at this point, and deserving of love. And there's also a very strong argument to make that this feature is a link feature, not just an image link feature, and as such should be an interface that is generic to all things that can be linked.

That should probably be the ultimate goal to strive towards, but in the mean time, it's probably okay to put this in the sidebar as the PR initially suggested, at least as a stepping stone towards greater things.

PR needs a rebase though.

karmatosed · 2018-10-29T18:36:59Z

+1 to @jasmussen's comment to focus us on time.

greatislander · 2018-10-29T20:35:21Z

Okay, I'll rebase.

greatislander · 2018-10-29T20:49:31Z

Now that #6316 is merged, should the user-supplied rel be additive?

E.g. if I set my image to open a link in a new window and then add a custom rel value, should the rel be set to noreferrer noopener myrelvalue?

getsource · 2018-10-29T22:12:37Z

Just a quick note that I've got a functioning version of this I can put in a separate branch if it's wanted. Was just figuring out how to best get it back into Github today.

Edit: I'll just go ahead and put it in a branch, because it was a significant amount of changes due to Gutenberg changes in the meantime. Will comment here when it's there.

getsource · 2018-10-29T23:01:32Z

It's referenced above, but I pushed the updated changes to this branch:
https://github.com/WordPress/gutenberg/tree/add/image-details

I currently only have the "Open in new tab" adding to rel if rel does not have content.

Merges PR #7771 to local branch to: - Bring up to date with the changes and moved files in Gutenberg - Update Tests for new expectations due to parser changes - Support rel defaults for "Open Link in New Tab" feature Props greatislander, getsource Co-authored-by: Ned Zimmerman <[email protected]>

greatislander · 2018-10-30T00:36:43Z

Rebased including a couple of tweaks from @getsource that I missed.

greatislander · 2018-10-30T01:25:59Z

Not sure why that one test is failing ¯_(ツ)_/¯

getsource · 2018-10-31T00:51:04Z

Thanks so much @greatislander!

Tested and seems to be working as expected.

This was passing locally, so I went ahead and re-ran the failed travis test, and it passed.

greatislander · 2018-11-01T13:44:56Z

I've followed the discussion in Slack and on #7504 around this; are there still concerns about the link class and rel sidebar functionality in this PR, or is it sufficient to address those two pieces of #7504?

getsource · 2018-11-01T18:03:08Z

@greatislander I believe I just misread @karmatosed comment on #7504, based on #7771 (comment).

@karmatosed, if that's the case, would you mind removing the change request? If not, we can definitely keep chatting about alternatives.

greatislander · 2018-11-06T01:47:42Z

Rebased again.

greatislander · 2018-11-07T14:27:41Z

Hi @karmatosed, can you review this again in the context of the discussion on the ticket? I think the build just needs to be restarted. Thanks!

greatislander · 2018-11-09T18:30:03Z

@talldan @karmatosed I've updated this PR with @talldan's suggestions, rebased… any chance of another review? See also @getsource's comment: #7771 (comment)

karmatosed

Let's get this in for now, we can iterate and review the media library itself in phase 2.

greatislander · 2018-11-13T13:18:43Z

Thanks @karmatosed! Should be merge-ready once tests pass.

greatislander · 2018-11-13T15:19:25Z

@talldan I've removed the attribute as per your comments— should be ready for another review and merge.

greatislander · 2018-11-15T16:07:43Z

@karmatosed @talldan @jasmussen Any chance this can be milestoned for 4.5 and merged? I've resolved all review comments. Thanks!

jasmussen · 2018-11-15T16:18:20Z

Yep, sorry about that.

talldan · 2018-11-16T01:21:02Z

Hey @greatislander - Thanks for addressing all those comments, and sorry for the delay responding. This does look good, I gave it a test and it works really nicely.

I don't think the issue with the linkClass working in raw transforms was ever addressed. It'd be great if that could be created as a separate issue or PR.

greatislander · 2018-11-16T16:47:30Z

Hi @talldan, thanks -- I will create a separate issue or PR for the linkClass issue.

greatislander · 2018-11-16T19:01:01Z

Follow-up ticket: #11995

youknowriad · 2018-11-19T10:07:15Z

packages/block-library/src/image/index.js

+ rel={ rel }
+ >
+ { image }
+ </a>


I wonder if this change creates an "invalid image" warning if you try to upgrade from an image with a link set to open a new tab.

checkout master

add an image with an external link

save

checkout this branch

refresh

The image shouldn't show up as invalid. I wonder if we should add a deprecated version for this?

To clarify: I didn't have the time to test it but it's a guess.

I took this for a spin, and things look like they are working as intended.

Screenshot from master:

Checked out this branch and reloaded:

I suspect the problem only occurs when you go the other way. Which is adding the attributes, then checking out a branch that removes them. And sure enough:

Going back to master:

But this shouldn't be a problem unless we choose to remove this feature :D

Checked again, this time I checkout master first, and set the open externally and added a custom link:

Checking out this branch and reloading still worked as expected:

👍 👍

Thanks for the tests @jasmussen :)

youknowriad

LGTM 👍

StaggerLeee · 2018-11-21T04:16:06Z

Don't forget new filter to make desired "Link to" default for all new images.

greatislander · 2018-11-21T14:23:06Z

@StaggerLeee That should be a separate issue; I agree that functionality is needed.

Ned Zimmerman added 5 commits July 7, 2018 10:26

Add link classes and rel attribute to image block (see WordPress#7504)

bde8135

Make labels consistent with classic editor.

bedb73c

Fix coding standards error

3c6f9da

Add tests

dfff33c

Consolidate check for presence of image link

c5bfce5

danielbachhuber added the Needs Design Feedback Needs general design feedback. label Jul 9, 2018

karmatosed self-requested a review July 13, 2018 17:53

karmatosed requested changes Jul 13, 2018

View reviewed changes

jasmussen removed the Needs Design Feedback Needs general design feedback. label Oct 11, 2018

Ned Zimmerman added 2 commits October 29, 2018 21:28

Rebase, incorporate upstream changes

41bd3d9

Merge branch 'upstream' into add/image-details

d538faf

antpb mentioned this pull request Oct 31, 2018

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

Closed

4 tasks

Merge branch 'master' into add/image-details

976f59d

Fix tests

38f669a

Merge branch 'upstream' into add/image-details

b4e4409

karmatosed approved these changes Nov 13, 2018

View reviewed changes

Ned Zimmerman added 2 commits November 13, 2018 08:12

Merge branch 'upstream' into add/image-details

df50d10

Resolve final review comment

48dc15f

Update CONTRIBUTORS.md

2b44182

jasmussen added this to the 4.5 milestone Nov 15, 2018

talldan approved these changes Nov 16, 2018

View reviewed changes

greatislander mentioned this pull request Nov 16, 2018

Raw transforms of linked image should support custom link classes #11995

Open

designsimply added [Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement. and removed [Type] Enhancement A suggestion for improvement. labels Nov 18, 2018

Merge branch 'upstream' into add/image-details

1b14c6c

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018

youknowriad reviewed Nov 19, 2018

View reviewed changes

youknowriad approved these changes Nov 19, 2018

View reviewed changes

gziolo merged commit dfa4e04 into WordPress:master Nov 19, 2018

greatislander deleted the add/image-details branch November 19, 2018 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add link rel and link class to image block inspector (see #7504) #7771

Add link rel and link class to image block inspector (see #7504) #7771

greatislander commented Jul 7, 2018 •

edited

Loading

karmatosed left a comment

greatislander commented Jul 13, 2018

karmatosed commented Jul 17, 2018

jasmussen commented Oct 11, 2018

karmatosed commented Oct 29, 2018

greatislander commented Oct 29, 2018

greatislander commented Oct 29, 2018

getsource commented Oct 29, 2018 •

edited

Loading

getsource commented Oct 29, 2018

greatislander commented Oct 30, 2018

greatislander commented Oct 30, 2018

getsource commented Oct 31, 2018

greatislander commented Nov 1, 2018

getsource commented Nov 1, 2018

greatislander commented Nov 6, 2018

greatislander commented Nov 7, 2018

greatislander commented Nov 9, 2018

karmatosed left a comment

greatislander commented Nov 13, 2018

greatislander commented Nov 13, 2018

greatislander commented Nov 15, 2018

jasmussen commented Nov 15, 2018

talldan commented Nov 16, 2018

greatislander commented Nov 16, 2018

greatislander commented Nov 16, 2018

youknowriad Nov 19, 2018

jasmussen Nov 19, 2018

jasmussen Nov 19, 2018

youknowriad Nov 19, 2018

youknowriad left a comment

StaggerLeee commented Nov 21, 2018

greatislander commented Nov 21, 2018

Add link rel and link class to image block inspector (see #7504) #7771

Add link rel and link class to image block inspector (see #7504) #7771

Conversation

greatislander commented Jul 7, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

karmatosed left a comment

Choose a reason for hiding this comment

greatislander commented Jul 13, 2018

karmatosed commented Jul 17, 2018

jasmussen commented Oct 11, 2018

karmatosed commented Oct 29, 2018

greatislander commented Oct 29, 2018

greatislander commented Oct 29, 2018

getsource commented Oct 29, 2018 • edited Loading

getsource commented Oct 29, 2018

greatislander commented Oct 30, 2018

greatislander commented Oct 30, 2018

getsource commented Oct 31, 2018

greatislander commented Nov 1, 2018

getsource commented Nov 1, 2018

greatislander commented Nov 6, 2018

greatislander commented Nov 7, 2018

greatislander commented Nov 9, 2018

karmatosed left a comment

Choose a reason for hiding this comment

greatislander commented Nov 13, 2018

greatislander commented Nov 13, 2018

greatislander commented Nov 15, 2018

jasmussen commented Nov 15, 2018

talldan commented Nov 16, 2018

greatislander commented Nov 16, 2018

greatislander commented Nov 16, 2018

youknowriad Nov 19, 2018

Choose a reason for hiding this comment

jasmussen Nov 19, 2018

Choose a reason for hiding this comment

jasmussen Nov 19, 2018

Choose a reason for hiding this comment

youknowriad Nov 19, 2018

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

StaggerLeee commented Nov 21, 2018

greatislander commented Nov 21, 2018

greatislander commented Jul 7, 2018 •

edited

Loading

getsource commented Oct 29, 2018 •

edited

Loading