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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'insert from url' for image block #9264

Merged
merged 24 commits into from Oct 18, 2018

Conversation

@talldan
Contributor

talldan commented Aug 23, 2018

Description

Closes #1536
Depends on #10495

This PR is split from #4155 into a separate chunk to make it easier to review. Hi-five to @imath for all his hard work on this change! 馃敟馃敟馃敟

Displays the 'Insert from URL' button for the image block. Makes the following changes:

  • Extracts a piece of commonly used code for detecting blob urls into the blob package
  • Allows the image block to use external urls for image src
  • Implements a URL Popover for editing the URL in the media placeholder
  • Changes the behaviour of the Edit image toolbar button when an external url is used. Clicking it toggles display of the placeholder so that the url can be changed

How has this been tested?

  • Added some unit tests
  • Tested manually adding and changing media urls

Screenshots

screen shot 2018-10-15 at 1 11 13 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

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

@talldan talldan self-assigned this Aug 23, 2018

@talldan talldan requested review from karmatosed and youknowriad Aug 23, 2018

@talldan talldan force-pushed the add/insert-from-url-for-image-block branch 3 times, most recently from bea42f8 to c079bc7 Aug 23, 2018

@imath

This comment has been minimized.

Contributor

imath commented Aug 23, 2018

Thanks a lot for your hi-five 馃槏& sorry #4155 has become quite a mess to fix.

@talldan

This comment has been minimized.

Contributor

talldan commented Aug 23, 2018

@imath Not at all - these things happen. We really appreciate the time you've spent on it.

@Soean Soean referenced this pull request Aug 23, 2018

Closed

Failed adding image via URL #9270

@karmatosed

This comment has been minimized.

Member

karmatosed commented Aug 23, 2018

Over having an input, could it be a button that opens to reveal an input? I do feel we're in danger of things getting a little complicated visually here. Before I mock that up what is the potential for that?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Aug 24, 2018

Over having an input, could it be a button that opens to reveal an input? I do feel we're in danger of things getting a little complicated visually here. Before I mock that up what is the potential for that?

It's important to note that this is using the same component as the video/audio/cover-image blocks to keep the same UI for all. If it's refactored, should it be done for all to keep them consistent?

@talldan

This comment has been minimized.

Contributor

talldan commented Aug 24, 2018

@brandonpayton also mentioned this morning that there would probably be issues when transforming to a gallery block from an image block with an external url, since the gallery block doesn't support external urls (at the moment it just ignores it and stays in the default state).

Probably the best thing to do would be to make gallery blocks support external urls, though the UI around that is a little more complex - there's less space here:
screen shot 2018-08-24 at 5 01 34 pm

We also might not be able to show external images in the edit gallery dialog:
screen shot 2018-08-24 at 5 02 47 pm

Another option would be to prevent the user from transforming to a gallery if they're using an external url, but that might be a bit weird since the user wouldn't know why.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Aug 25, 2018

It's important to note that this is using the same component as the video/audio/cover-image blocks to keep the same UI for all. If it's refactored, should it be done for all to keep them consistent?

Yes I think we should do my change for all. Apologies for dropping in and causing a change of direction but I think it would be better.

@talldan talldan force-pushed the add/insert-from-url-for-image-block branch from c76aadf to 8934feb Sep 12, 2018

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 12, 2018

Hey @karmatosed! I've added a button to expand (and collapse) the url input. Here's a gif:
enter-url

Let me know what you think 馃槃

@karmatosed

This comment has been minimized.

Member

karmatosed commented Sep 13, 2018

Thanks @talldan I think that's getting there. My only concern is the arrow down indicates a down action but it opens to the side. What other icons could be used to indicate this?

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 13, 2018

I tried the right facing solid arrow, but it looks like a play button, so I've switched it to the chevron:
expand-collapse

Not sure if this is still quite right. Maybe we need something like this:
https://iconscout.com/icon/expand-arrow-7

There isn't anything like that in dashicons at the moment, so would need to add something new.

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 21, 2018

Hey @karmatosed - if you get a chance, please could you take a look at the updated version of this in the gif above.

I'm still not sure the arrows are quite right, let me know what you think.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Sep 21, 2018

@talldan thanks for the work. I am so torn on the arrows here. They are better but because we don't use them anywhere else I am starting to get nervous about adding a new pattern. Let's get a second opinion! @jasmussen what do you think?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Sep 24, 2018

So, how about we take a different approach?

Search fields can do magical things on focus. Like expand. So we could do this on the initial placeholder:

                               Image
         [ 鈫 Upload]  [Media Library] [ Add from URL... ]

Note how the search field is no wider than the text placeholder label.

Then when you set focus in the search field, we do this:

                               Image
                [ 鈫 Upload]  [Media Library] 
[ |Add from URL                                      ] [Use URL]

You can even do something like hide the "Use URL" button by default, and then do something like:

.url-input:focus + .url-input__button { display: inline-flex; } to unhide the button on focus.

A separate button to expand/collapse feels like a headache we don't want.

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Sep 24, 2018

Just to throw out an alternate idea, what if there's an "Insert from URL" button that opens the modal with an "insert from URL" tab selected and have the UI there?

edit_page_ chris_van_patten _wordpress

Adds a little extra consistency too so the "Insert from URL" action is available in the modal, like the other two actions (upload/library).

@talldan talldan force-pushed the add/insert-from-url-for-image-block branch from 4796537 to 66f5215 Sep 24, 2018

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 24, 2018

Hi @jasmussen - thanks for the feedback. I had a test, and one thing to note is that if there's an input in a block, it becomes focused when the block is added. That causes the URL input to be expanded by default, which is undesirable.

I tried a slightly different approach - having the url expand/collapse base on whether the input has any text entered. Here's a gif:
url-input-expand

IMO, it feels a little jarring that the input suddenly moves unexpectedly.

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 24, 2018

@chrisvanpatten That seems like a good suggestion. I'll have to look into it. The modal itself is non-gutenberg code, so I'll have to track down where it comes from and what it's capable of.

Will also have to make sure it works with the contributor role, as that's one of the aims for inserting via a url.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Sep 24, 2018

Yep, a button that opened the existing UI could be a decent solution for now.

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Sep 24, 2018

@talldan in the non-Gutenberg media modal it shows up in the left panel:

add_new_page_ paleo_meal_plans _wordpress

add_new_page_ paleo_meal_plans _wordpress

Not sure what would be required to make it a top tab though, instead of a side nav option.

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 25, 2018

@chrisvanpatten I had a look into it but without much success. Not a lot of documentation for wp.media as far as I can tell. There's a very old Stack Exchange thread where a similar thing is discussed:
https://wordpress.stackexchange.com/questions/92096/how-can-i-add-the-insert-from-url-tab-to-a-custom-3-5-media-uploader

To apply the idea being discussed in that link to gutenberg would require quite a big rewrite of our media upload code - quite a lot of code for not much gain.

What are the alternatives? We could always show a normal <Modal> with the form instead? Or explore other ways to include this in the MediaPlaceholder body.

@gziolo gziolo force-pushed the add/insert-from-url-for-image-block branch from 99aafbf to 72f302a Oct 18, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

I rebased with master to resolve one conflict. I will retest and merge, thanks for addressing feedback so quickly 馃挴

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

It works properly 馃帀

It might need some more tweaks to improve the flow when user select an image from media library. You can swap it only for another image from the library. You can't replace it with an external URL anymore.

When editing an external URL, I have to click on Insert from URL to see the previously used URL:
screen shot 2018-10-18 at 17 32 48

Maybe we could put an image as a background to the MediaPlaceholder and use this state as a starting point for editing. Something like this:
screen shot 2018-10-18 at 17 40 31

I also marked Insert from URL to indicate that it sources the image.

@gziolo gziolo merged commit 9066b8c into master Oct 18, 2018

2 checks passed

codecov/project 49.43% (-0.03%) compared to f1d163a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the add/insert-from-url-for-image-block branch Oct 18, 2018

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 22, 2018

Adding an image through the URL then clicking the pencil/edit button and then deselecting the image block the media upload screen is still seen. Deselecting the image block should then remove the media upload message and show the image.

deslect-image-block-should-image

@talldan

This comment has been minimized.

Contributor

talldan commented Oct 23, 2018

Thanks for the comment @paaljoachim. This PR didn't change that functionality, it's worked the same way in the audio block since before this change, so I think you'd need to create a new issue to cover any changes you think need to be made.

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 23, 2018

Thanks Daniel! @talldan

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

Add 'insert from url' for image block (WordPress#9264)
* Extract isBlobURL into blob package and update usage

Co-authored-by: imath <math.viet@gmail.com>

* Add ability to insert an image in image block using a url

Co-authored-by: imath <math.viet@gmail.com>

* Position url input underneath upload file buttons and apply margin

Co-authored-by: imath <math.viet@gmail.com>

* Refactor styles to avoid use of element selector

Co-authored-by: imath <math.viet@gmail.com>

* Ensure form elements have bottom margin to separate them on tiny screens

Co-authored-by: imath <math.viet@gmail.com>

* Change type of block used in e2e test to a separator

As mentioned in the comment above, this test will not work for blocks that
contain an input. The image block now contains a url input field causing
the test to fail.

Co-authored-by: imath <math.viet@gmail.com>

* Fix invalid reference to this.isBlobURL

* Add expandable section for url form on media placeholder

* Change icon for media placeholder button to left/right chevron

* Update snapshots

* Try: expanding url input based on whether text is entered

* Try: use link popver for Insert from URL in media placeholder

* Implement URLPopover in MediaPlaceholder

* Use consistent declaration of defaults for props

* Use a generalized selector for buttons in the media placeholder

* Improve copy in URL input placeholder

* Make behaviour when selecting a differnent url the same as other media blocks

* Update snapshot tests

* Revert "Change type of block used in e2e test to a separator"

This reverts commit 6c8546c.

* Use onClose over onClickOutside (onClose implements onClickOutside)

* Rename functions

* Address code review feedback

* Update changelog for blob package

* Updated changelog for editor package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment