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 link rel and link class to image block inspector (see #7504) #7771

Merged
merged 16 commits into from Nov 19, 2018

Conversation

Projects
None yet
10 participants
@greatislander
Contributor

greatislander commented Jul 7, 2018

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

Screen shot of image block inspector with link class and link rel fields added.

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.

greatislander added some commits Jul 7, 2018

@karmatosed karmatosed self-requested a review Jul 13, 2018

@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

This comment has been minimized.

Contributor

greatislander commented Jul 13, 2018

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

This comment has been minimized.

Member

karmatosed commented Jul 17, 2018

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:

artboard

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 11, 2018

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

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

This comment has been minimized.

Member

karmatosed commented Oct 29, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Oct 29, 2018

Okay, I'll rebase.

@greatislander

This comment has been minimized.

Contributor

greatislander commented Oct 29, 2018

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

This comment has been minimized.

Contributor

getsource commented Oct 29, 2018

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 added a commit that referenced this pull request Oct 29, 2018

Merge PR #7771 to branch.
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 <ned@bight.ca>
@getsource

This comment has been minimized.

Contributor

getsource commented Oct 29, 2018

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.

getsource added a commit that referenced this pull request Oct 29, 2018

Merge PR #7771 to branch.
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 <ned@bight.ca>
@greatislander

This comment has been minimized.

Contributor

greatislander commented Oct 30, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Oct 30, 2018

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

@getsource

This comment has been minimized.

Contributor

getsource commented Oct 31, 2018

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

This comment has been minimized.

Contributor

greatislander commented Nov 1, 2018

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

This comment has been minimized.

Contributor

getsource commented Nov 1, 2018

@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

This comment has been minimized.

Contributor

greatislander commented Nov 6, 2018

Rebased again.

@greatislander

This comment has been minimized.

Contributor

greatislander commented Nov 7, 2018

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

This comment has been minimized.

Contributor

greatislander commented Nov 9, 2018

@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

This comment has been minimized.

Contributor

greatislander commented Nov 13, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Nov 13, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Nov 15, 2018

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

@jasmussen jasmussen added this to the 4.5 milestone Nov 15, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 15, 2018

Yep, sorry about that.

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 16, 2018

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

This comment has been minimized.

Contributor

greatislander commented Nov 16, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Nov 16, 2018

Follow-up ticket: #11995

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

This comment has been minimized.

@youknowriad

youknowriad Nov 19, 2018

Contributor

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.

This comment has been minimized.

@jasmussen

jasmussen Nov 19, 2018

Contributor

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

Screenshot from master:

screenshot 2018-11-19 at 11 32 48

Checked out this branch and reloaded:

screenshot 2018-11-19 at 11 33 05

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:

screenshot 2018-11-19 at 11 34 52

Going back to master:

screenshot 2018-11-19 at 11 35 16

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

This comment has been minimized.

@jasmussen

jasmussen Nov 19, 2018

Contributor

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

screenshot 2018-11-19 at 11 41 03

Checking out this branch and reloading still worked as expected:

screenshot 2018-11-19 at 11 41 35

👍 👍

This comment has been minimized.

@youknowriad

youknowriad Nov 19, 2018

Contributor

Thanks for the tests @jasmussen :)

@youknowriad

LGTM 👍

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@greatislander greatislander deleted the greatislander:add/image-details branch Nov 19, 2018

@StaggerLeee

This comment has been minimized.

StaggerLeee commented Nov 21, 2018

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

@greatislander

This comment has been minimized.

Contributor

greatislander commented Nov 21, 2018

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

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

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

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

* Make labels consistent with classic editor.

* Fix coding standards error

* Add tests

* Consolidate check for presence of image link

* Rebase, incorporate upstream changes

* Changes as per review

* Fix tests

* Resolve final review comment

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