Skip to content
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

Link images in media + text #18139

Merged
merged 16 commits into from Dec 18, 2019
Merged

Link images in media + text #18139

merged 16 commits into from Dec 18, 2019

Conversation

@draganescu
Copy link
Contributor

draganescu commented Oct 28, 2019

Description

Adds a link functionality to images in the media + text block.
Closes: #15422

Screenshots

Screenshot 2019-11-01 at 15 59 41

How has this been tested?

Tested locally.

Types of changes

Copied the same functionality available in the 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.
  • I've included developer documentation if appropriate.
@draganescu draganescu force-pushed the add/link-to-media-text branch from caa6e63 to 9dcf7ed Nov 1, 2019
@draganescu draganescu changed the title Link media in media + text Link images in media + text Nov 1, 2019
@draganescu draganescu marked this pull request as ready for review Nov 1, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 1, 2019

@mapk @paaljoachim I effectively copied the stuff in the image block.
What should I remove, because everything there seems like a good idea?

@brezocordero

This comment has been minimized.

Copy link

brezocordero commented Nov 3, 2019

Tested it visually and it works as expected.
Gutenberg 6.8.0
Chrome Version 77.0.3865.120 (Official Build) (64-bit)

AddLinkToMediaText

@Dabalina

This comment has been minimized.

Copy link

Dabalina commented Nov 3, 2019

Tested on Chrome Version 78.0.3904.70 (Official Build) (64-bit) and Gutenberg 6.8.0

I was able to add the link to the image however the open in new tab is not working.

ezgif com-optimize

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Nov 8, 2019

I just tested this as well and noticed a discrepancy between this link interaction and a text link interaction. Here's what I'm seeing.

The unlink interaction on a Media + Text block.

Screen Shot 2019-11-07 at 6 18 39 PM

The unlink interaction on a text link.

Screen Shot 2019-11-07 at 6 19 23 PM

It would be great to keep these interactions consistent.

@phpbits

This comment has been minimized.

Copy link

phpbits commented Nov 10, 2019

@draganescu I've added this feature on EditorsKit plugin as well. I've been asked by @paaljoachim to check this PR and here are my concerns:

  1. Would it be better to use Toggles on "rel" as well similar to EditorsKit?
  2. I've added option to assign link to whole block aside from the image. It's personal preference but I would love to hear your idea about his as well.
  3. What will be the impact of the upcoming Link interface?

Thanks a lot!

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Nov 10, 2019

Here is a link to a gif from EditorsKit: #15422 (comment)

@phpbits

This comment has been minimized.

Copy link

phpbits commented Nov 10, 2019

I just tested this as well and noticed a discrepancy between this link interaction and a text link interaction. Here's what I'm seeing.

The unlink interaction on a Media + Text block.

Screen Shot 2019-11-07 at 6 18 39 PM

The unlink interaction on a text link.

Screen Shot 2019-11-07 at 6 19 23 PM

It would be great to keep these interactions consistent.

I'm not sure if this is a good idea. What if I want to just edit the link? Having it as "Edit" instead of "Remove" will give much more control than doing the same process again when editing the link. Thanks!

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Nov 12, 2019

I'm not sure if this is a good idea. What if I want to just edit the link? Having it as "Edit" instead of "Remove" will give much more control than doing the same process again when editing the link. Thanks!

Your comment had me review how an Image block works here. Thanks for that! Linking an Image block looks like this:

edit-image 2019-11-11 18_50_14

Because the Media+Text block is linking an image, let's mimic the Image block flow here too. I withdraw my earlier comment. 😉

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 12, 2019

Thanks for the help @phpbits 👍
Good points on your concerns too:

Would it be better to use Toggles on "rel" as well similar to EditorsKit?

I would go more for a consistent experience even if the toggles do make more sense. Plus, with the new LinkEditor UI this might be completely different! So since this functionality is copied from the image block it should be the same manner of setting the rel attrs. too.

I've added option to assign link to whole block aside from the image. It's personal preference but I would love to hear your idea about his as well.

My answer is I don't know, as this, being a core block, should not provide all the possible options, nor replace the need for more complex and/or specific 3rd party blocks, based on the same media + text content. @mapk any suggestions on this?

What will be the impact of the upcoming Link interface ?

The new link interface will replace the whole flow of all the blocks which now have the URLPopover, LinkViewer and LinkEditor as part of their flow. So this iteration on media+text might be short lived. However, at least users will get the option now :)

@phpbits

This comment has been minimized.

Copy link

phpbits commented Nov 13, 2019

My answer is I don't know, as this, being a core block, should not provide all the possible options, nor replace the need for more complex and/or specific 3rd party blocks, based on the same media + text content.

How about adding slotfill to let us add option in there. Would be really helpful if I can add that toggle there instead of removing what you've added and register my own. Thanks!

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Nov 13, 2019

Hi @phpbits,

How about adding slotfill to let us add option in there. Would be really helpful if I can add that toggle there instead of removing what you've added and register my own. Thanks!

There is an ongoing discussion related to Link settings extensibility #13190. I guess it may be helpful if you share your use cases there, so we have a central place where we can discuss this issue.

@phpbits

This comment has been minimized.

Copy link

phpbits commented Nov 13, 2019

There is an ongoing discussion related to Link settings extensibility #13190. I guess it may be helpful if you share your use cases there, so we have a central place where we can discuss this issue.

@jorgefilipecosta Is this the right one? I've commented on other discussion about the Link Interface and extensibility. I'm confused right now in which is the main discussion about this. Thanks!

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Nov 13, 2019

@jorgefilipecosta Is this the right one? I've commented on other discussion about the Link Interface and extensibility. I'm confused right now in which is the main discussion about this. Thanks!

It seems there was another related issue #16609 but is now closed. The PR #13190 is still open so I thought it could be a good place.

@phpbits

This comment has been minimized.

Copy link

phpbits commented Nov 13, 2019

@jorgefilipecosta Here's the issue I've created #17556. There seems to be overlap with the upcoming Link Interface. I'm not sure which will be the global link components. Thanks!

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Nov 13, 2019

@jorgefilipecosta Here's the issue I've created #17556. There seems to be overlap with the upcoming Link Interface. I'm not sure which will be the global link components. Thanks!

I was not aware of that issue. Given that the issue is still open, I guess it can be a good place to continue the discussion. Given that the link UI currently does not have a fill I guess this one should also not add it. But if we decide to add it to normal link UI we should also add it to this one.

@draganescu draganescu requested review from ellatrix and youknowriad as code owners Nov 14, 2019
@draganescu draganescu force-pushed the add/link-to-media-text branch from 6bce3b8 to 9a61d11 Dec 9, 2019
@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Dec 9, 2019

Not related to this PR or a blocker but the number of different UIs we have for inserting a link is getting unnerving!

So true. I think keeping this PR consistent with how to add a link on an Image block is the right path forward for now.

I've added option to assign link to whole block aside from the image. It's personal preference but I would love to hear your idea about his as well.

I'd rather hold off on this right now. This particular block isn't always visually identified as a "block" on the frontend, it's just text next to an image. Allowing a button or link in the text works, linking the image works, but linking the whole block could lead to confusion in my opinion.

@caostar

This comment has been minimized.

Copy link

caostar commented Dec 13, 2019

Hello guys. Trying to understand if this is happening soon or if I should change the theme we created here. @mapk comment seems to indicate that this merge will not happen any time soon. Is that correct?

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Dec 16, 2019

No, @caostar I think @mapk meant to hold off on adding a link to the whole block, but this change should land asap, just adding alink to the image in media + text.

Waiting on a new review :)

@caostar

This comment has been minimized.

Copy link

caostar commented Dec 16, 2019

Tks @draganescu !
Will wait then :)

Copy link
Member

jorgefilipecosta left a comment

I'm facing a bug. On the media&text block if I select "Media File" and then I replace the image the block continues to link to the previous image. On the image block, the problem is not happening.

@@ -0,0 +1,312 @@
/**

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 16, 2019

Member

Not a blocker for this PR, but I think we should create a readme and add some test cases for this component.

This comment has been minimized.

Copy link
@draganescu

draganescu Dec 17, 2019

Author Contributor

I think this will be deprecated by the new LinkEditor.

@mapk
mapk approved these changes Dec 17, 2019
Copy link
Contributor

mapk left a comment

Looks good from a design standpoint. 👍

Co-Authored-By: Jorge Costa <jorge.costa@automattic.com>
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Dec 17, 2019

Yes I can repro the bug Jorge found, working to fix it.

Copy link
Member

jorgefilipecosta left a comment

LGTM 👍

@draganescu draganescu merged commit 0332f5b into master Dec 18, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@draganescu draganescu deleted the add/link-to-media-text branch Dec 18, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
>
{ ( ! url || isEditingLink ) && (
<URLPopover.LinkEditor
className="editor-format-toolbar__link-container-content block-editor-format-toolbar__link-container-content"

This comment has been minimized.

Copy link
@aduth

aduth Jan 7, 2020

Member

This is not a valid class name for this component:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#naming

It violates the guideline to not inherit styles from other compoennts:

A component's class name should never be used outside its own folder (with rare exceptions such as _z-index.scss). If you need to inherit styles of another component in your own components, you should render an instance of that other component. At worst, you should duplicate the styles within your own component's stylesheet. This is intended to improve maintainability by isolating shared components as a reusable interface, reducing the surface area of similar UI elements by adapting a limited set of common components to support a varied set of use-cases.

It also introduces additional legacy compatibility class names intended to have been removed as part of #19050 (though I also note that this is problematic in the format-library from which this code was presumably copied).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.