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

Add link UI with autocomplete to the image block #15570

Merged
merged 1 commit into from Jul 3, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented May 10, 2019

Description

Fix: #8265

This PR implements a button in the image block that allows users to use the link UI already available in the paragraph.
Some components were extracted from the link format, to make them available in the image block.

How has this been tested?

I added an image block.
I selected an image.
I pressed the link button in the toolbar.
I tried to add a link, I verified I can use autocomplete; I selected a link; I verified the link was applied.
I verified in the block inspector that the "Link To" field was changed to "Custom Url".

@ajitbohra
Copy link
Member

left a comment

Functionality wise works as advertised 👍

One observation when switching option from media/attachment to custom it uses the value of media/attachment.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I think we went back and forth on this one in previous releases.
It works well now but I wonder about the UI. It feels inconsistent to have some links settings in the inspector and others in the sidebar. I pinged some design eyes.

@youknowriad youknowriad requested review from jasmussen, kjellr and mapk May 13, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Thanks for this, very nice work. Overall GIF:

linkui

The concern that Riad highlights is, correct me if I'm wrong, summarized in this GIF:

sidebar

The concern is that it's duplicate UI. Usually when there are two ways to accomplish the same, it begs a question of the user: which way is the right way?

Note that this is different from redundant UI, which is also duplicate, but is identical.

This challenge is exactly why I was a bit sad to see the old image link UI being moved to the sidebar as part of what is currently in master. I knew that it was a temporary stopgap to ensure feature parity, but that the UI implemented was not very good because it had to happen fast. This is where it bites us. Not saying that to disparage, just as context — I've been around for the ride, and sometimes you have to take detours, and I'm happy we're now looking at it again.

To resurrect that discussion a little bit — linking an image feels like a basic operation. Following our principles, that makes it primary UI, which means it should not be in the sidebar. For that reason, I quite like that it's now a primary operation, and the fact that it's similar to the paragraph UI is good. Technically the two also work nicely side by side — one is aware of the other and you can go about your day. As such, I have no objection to this PR being merged as is, so long as we ticket improvements to the sidebar link UI.

However those sidebar improvements are important. As a strawman, this seems like it would be an improvement:

mockup

At least then, the two UI's would be unified into one.


I also noticed something interesting:

Screenshot 2019-05-13 at 11 28 27

Notice the extra small 1px gray border around the formatting buttons — that's a regression. It's also in master, so it's not the fault of this branch, but it is just more visible here because the button pops out a popover and the extra border lingers there. I'll investigate.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

PR for the lingering outline thing in #15592

@talldan

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

However those sidebar improvements are important. As a strawman, this seems like it would be an improvement

Some thoughts on the design ... The user's interaction with with the link popover naturally flows:

  1. Enter a URL
  2. Change the settings for that URL (Open in new tab etc.)

With the 'Link To' option that order doesn't make sense because 'Link To' modifies the URL that was initially entered. In the sidebar's link settings, it's the first step, which makes sense to me. I wonder if there's another way to express that setting in the popover version that would allow the user to have a more natural editing flow. 🤔

edit:
Some ideas. The 'Attachment Page' and 'Media File' options for 'Link To' could also become toolbar buttons alongside the existing button for specifying a Custom URL.

Alternatively, maybe this link popover doesn't have any additional settings, so that there's less duplication.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

With the 'Link To' option that order doesn't make sense because 'Link To' modifies the URL that was initially entered. In the sidebar's link settings, it's the first step, which makes sense to me. I wonder if there's another way to express that setting in the popover version that would allow the user to have a more natural editing flow. 🤔

This is a good point, and speaks to the existing limitations of the legacy UI. It definitely needs a re-think overall. I don't think additional buttons are the solution — in my mind "attachment page" and "media file" have more in common with auto-complete suggestions than they do modal options for the entire link interface. They're all just links. My suggestion for putting it in the advanced dropdown for now was more to at least group the link UIs together so we can keep moving.

@kjellr do you have any magical insights here that eldude me? There could be some simple elegant solution I'm not seeing.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

What if we add the Link to option to the popover before the URL input? The open in a new tab would still be an andavanced option.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Noting that if you rebase, that lingering focus thing is gone :) — if not, it will be gone post merge.

What if we add the Link to option to the popover before the URL input? The open in a new tab would still be an andavanced option.

Can you elaborate a little bit?

As mentioned, I don't feel it necessary to re-do the link UI to be able to ship THIS PR, if we mean to ship it quickly. Just ticket the improvement and we can move.

However if there is bandwidth for a re-think, I would suggest that Google Docs has a solid interface:

docs

It is a single link UI, but it also shows contextual options at the bottom of the dropdown. Just press tab in that link dialog to access those options.

The equivalent for us is almost there already: you can search for an existing page to link to, and an "autocomplete-esque" UI will pop out. We could do so that when you invoke the link dialog on the image, that autocomplete UI is already open, and shows two options at the bottom: link to attachment page, or link to media file. The keyboard flow would be this:

  • Tab to link button on toolbar, press enter.
  • Focus is set in inputfield. Autocomplete box with the additional options are already open.
  • Type URL and autocomplete box closes. Press enter to apply URL.
  • OR: type a page or post title to invoke the search. Use arrow keys to choose page, press enter to apply link to page or post.
  • OR: use arrow keys or press tab right off the bat to choose "link to media file" or "link to attachment page".

Does that make sense? Need a mockup?

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

The equivalent for us is almost there already: you can search for an existing page to link to, and an "autocomplete-esque" UI will pop out. We could do so that when you invoke the link dialog on the image, that autocomplete UI is already open, and shows two options at the bottom: link to attachment page, or link to media file.

Yep, I fully endorse this. 🙌 I actually mocked this up a while back and just dug it up:

Frame

Also, is it possible for us to be smart and say "Link to image file" here instead of "media file"? I think that will be more clear to folks.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Killer mockup!

I do think a separator and some icons next to those two options will give them slightly more presence, but the idea feels solid.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Yeah — I figured we'd still be using the same component we're used to here, right? If so, we don't usually put icons in there.

For the separator, were you thinking that'd be a separator between these and other options in the dropdown (once they appear)? I'd figured that these options would just disappear once you typed something else, since they're not technically auto-completes.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

An absent icon wouldn't block anything, but it would make it less "missable" — right now it's a little invisible down there.

For the separator, were you thinking that'd be a separator between these and other options in the dropdown (once they appear)? I'd figured that these options would just disappear once you typed something else, since they're not technically auto-completes.

Good point. Yes those options available right off the bat would disappear as soon as you type, so no need to separate them.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Here's a revised mockup with icons. Not totally sure what to use for the Media file one — theoretically it makes sense to use the image icon, but then it's also repeating the block icon nearby, which may be confusing.

Frame

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch from 927e84f to eb41254 May 13, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

The UI was updated yesterday to follow the mockups and can be tested. I felt the need to add some UI elements like a button that allows the user to remove the link (now we don't have the option to set the link to none).
The duplicate UI was removed.
The only thing missing are the icons, yesterday I did not see them. I will try to add the icons soon.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Just gave this one last test, and I have a quick design question: when there is a link added, should the block use the "unlink" icon in the toolbar, like we do in the paragraph block?

Hi @kjellr, I don't we should follow the same approach, in the RichText formats, we can do that because when the cursor is on a link the popover is automatically open. In the image case, I don't think the popover should automatically open when the image is selected. So if we added the unlink button if I added a link and then I wanted to change the link rel, I would need to click the unlink button the link would be removed, I would then click the link button add the link again and select the correct rel. That seems an un-optimal, that was the reason why I did not follow the unlink button approach and opted to add an explicit button to remove the link in the popover which is not available in the paragraph.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

In the image case, I don't think the popover should automatically open when the image is selected. So if we added the unlink button if I added a link and then I wanted to change the link rel, I would need to click the unlink button the link would be removed, I would then click the link button add the link again and select the correct rel. That seems an un-optimal, that was the reason why I did not follow the unlink button approach and opted to add an explicit button to remove the link in the popover which is not available in the paragraph.

That's great reasoning, thanks. I think we're all set with design feedback for now, so I'll remove the Needs Design Feedback label. 🎉

One small a11y note: I expected to be able to be able to use the down arrow to get into the Media File and Attachment Page options. But tab worked fine instead, so I think we're ok. I'd love a11y team feedback on that, and the updated UI in general.

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch from 888f4bf to 12a6cea Jun 5, 2019

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Discussed during today's accessibility bug scrub. At a first glance, anything that moves us away from the concept of “Sidebar” sounds good 🙂

Needs to be tested for implementation details. Keeping the "Needs a11y feedback" label for now.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Additional note: One more concern is consistency. Not just for accessibility, also for general usability. These “Link” buttons do very different things across the various blocks: Image, Video, Audio, etc.

Not sure forcing users to learn a new UI each time is the best user experience. On the other hand, if this pattern turns out to work well, it could be considered also for other blocks.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Discussed during today's accessibility bug scrub. At a first glance and looking at the GIFs this seems a step in the right direction, as it moves the UI link from the sidebar (not so accessible) to the block. Having the relevant UI pieces "in place" within the block sounds sensible.

Would need some testing with assistive technologies and some real user testing. WCEU could be a good opportunity.

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch 3 times, most recently from bbbe4d3 to ec7b647 Jun 19, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch from ec7b647 to c0d7d38 Jul 1, 2019

@jorgefilipecosta jorgefilipecosta requested a review from etoledom as a code owner Jul 1, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

I did some tests on this PR with voice over and things worked as expected. I guess this PR should be ready merge.

@kjellr

kjellr approved these changes Jul 2, 2019

Copy link
Contributor

left a comment

👍 This works well! Thanks, @jorgefilipecosta!

</div>
) }
</Popover>
);
}
}

const LinkEditor = ( {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 2, 2019

Contributor

I think we need to reorganize all the link related comments better. For example why is this in the popover? It can be used without it right? Also the relationships between all these components is not obvious. we should probably have a README for each one of these components and organize them in a common folder or something like that.

Makes me wonder if we should mark all these things as "Experimental" before doing the reorganization.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 2, 2019

Author Member

Yes, I think it may be possible to use these components outside the URLPopover, but they are precise UI pieces, and the most probable use case is inside a URLPopover component.
I don't think we should expose these components directly as part of wp.components otherwise we populate the module with particular UI pieces, I agree we should mark these components as experimental until we decide on a more profound reorganization.

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch from c0d7d38 to bbf72fe Jul 3, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/link-ui-with-auto-complete-to-image branch from bbf72fe to 2e30904 Jul 3, 2019

@jorgefilipecosta jorgefilipecosta merged commit 2582944 into master Jul 3, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/link-ui-with-auto-complete-to-image branch Jul 3, 2019

@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 5, 2019

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

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.