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

Update on Gallery Block. Replaced FormFileUpload with MediaPlaceholder. #9682

Closed

Conversation

kevinbazira
Copy link

@kevinbazira kevinbazira commented Sep 7, 2018

PROBLEM

At the beginning
Empty gallery block gives you 2 options;
1. Upload images from computer
2. Use images in media library

In case you want to add more images
Used gallery block gives you 1 option;
1. Upload images from computer

SOLUTION

In case you want to add more images
Used gallery block now gives you 2 options;
1. Upload images from computer
2. Use images in media library

Solution to issue: #8309

Types of changes

Replaced FormFileUpload with MediaPlaceholder.

Checklist:

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

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that now that the "Add image" to Gallery is always shown after the gallery's content, it makes sense to use the MediaPlaceholder for it.

I'm adding a Design Feedback label to confirm.

@@ -142,6 +138,21 @@ class GalleryEdit extends Component {
} );
}

onAddMoreImages( newImages ) {
if ( ! window.event ) { // from "Upload" option after newImages absolute paths have been generated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain I understand why do we need this if else and I don't think having two separate functions is really needed here unless I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad thanks for the feedback.

Please see response to your questions below;

1. why do we need this if else
Without the if else, when one adds an image to the gallery using the MediaPlaceholder "Upload" button, 2 images are added instead of 1 causing duplicity.
We noticed this was because, during the file upload 2 steps happen. Step1: A temporary path is created. Step2: An absolute path is created after image is sucessfully uploaded. These 2 paths were both being picked and added to the gallery.
To stop the duplicity we used the if else to confirm that the upload event is complete and that's when we pick the new image's absolute path and add it to the gallery.

2. I don't think having two separate functions is really needed here unless I'm missing something?
We used two separate functions onAddMoreImages() and updateImagesAttribute() for code readability and ease debugging. Also in the future, one might want to call updateImagesAttribute() without onAddMoreImages().

@youknowriad youknowriad added [Feature] Media Anything that impacts the experience of managing media Needs Design Feedback Needs general design feedback. labels Sep 12, 2018
@jasmussen
Copy link
Contributor

This is a legitimate use case, thanks so much for working in this and submitting a pull request. Screenshot:

screen shot 2018-09-12 at 13 18 27

However I'm not sure we should use the actual placeholder in there. The placeholder is implied to be be replaced by a block, whereas in this case you aren't replacing a block, you are adding to a block.

But the use case here is super real, and something we should address.

There is a separate issue going on in #8589 (comment) which aims to improve the various "appenders" we have. There's a mockup for a new style for adding to galleries as well, but we should definitely split that in two buttons — one for uploading, one for adding from the library.

@karmatosed
Copy link
Member

However I'm not sure we should use the actual placeholder in there. The placeholder is implied to be be replaced by a block, whereas in this case you aren't replacing a block, you are adding to a block.

I would second this, let's iterate what we show but the addition of this as functionality is a good idea, thanks.

@kjellr
Copy link
Contributor

kjellr commented Sep 26, 2018

Adding a couple potential comps to help move this along. These are similar to the styles in #8589.

An option with two buttons:

artboard copy

Though, I'd suggest changing the + to an upload icon in this case to be more clear:

artboard copy 2

I'm not totally sure about just using "Media Library" as the label for this second button. To match up with the other label, it should probably be more active. Maybe "Add from Media Library".

(We could try this without any text labels, but I think it's necessary to help sort out the meaning behind the two separate buttons)

@jasmussen
Copy link
Contributor

@kjellr see also work ongoing in #10136.

I like the visual of what you're doing with two buttons. But I feel it's ever so slightly misleading, as the purpose of the dashed outline is to indicate "an image will land here". And with two buttons it feels like an image will land on the right, but not on the left.

@kjellr
Copy link
Contributor

kjellr commented Sep 27, 2018

But I feel it's ever so slightly misleading, as the purpose of the dashed outline is to indicate "an image will land here".

Great point:

bitmap

Though, is the dashed icon button also supposed to be clickable or just a drop area? I realized I'm not entirely clear. (Might be irrelevant — as of #10136 it sounds like we're no longer pursuing this dotted border in general. 😄)

The gallery block is a weird, unique case: at a high level, it's akin to a parent/child block relationship, except that the gallery only accepts a single block type. Crucially though, each image within a gallery is not actually a block.

It seems like we either need a one-off solution (like the comp above), or we need to rebuild the block so that it relies on nested blocks, and can use a more standard sibling inserter as in #10136. Does that seem right?

@jasmussen
Copy link
Contributor

Yes I think it should also be a dropzone. And we are still pursuing the dashed outline :D #10136 (comment)

And yes, I agree, the gallery block is definitely a bit of an edgecase.

Can't we just put two buttons inside the dropzone area though? A click on the background and upload do the same, a click on "media library" opens that.

@kjellr
Copy link
Contributor

kjellr commented Sep 28, 2018

And we are still pursuing the dashed outline :D #10136 (comment)

Ah, cool — I misread that. Thanks for clarifying.

Can't we just put two buttons inside the dropzone area though? A click on the background and upload do the same, a click on "media library" opens that.

Definitely. Here are a handful of explorations around that (my preference is number 1 or 2):

artboard

artboard copy 4

artboard copy 5

artboard copy 2

artboard copy

@jasmussen
Copy link
Contributor

Nice! I like all of these but I love 1, 2, and 4. @karmatosed?

@karmatosed
Copy link
Member

I think 2 seems more understandable to me.

@chrisvanpatten
Copy link
Contributor

I like 2 a lot :)

@talldan
Copy link
Contributor

talldan commented Oct 5, 2018

Options 2-5 would definitely make supporting an 'Insert from URL' button in the gallery appender a bit easier. This is how we're looking to change it in the media placeholder - #9264 (comment)

@karmatosed
Copy link
Member

Let's go with 2 as a movement forward.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@ocean90
Copy link
Member

ocean90 commented Nov 11, 2018

IMO only option 3 is the most understandable solution. It's already known from the media placeholders but more important, both options are clearly visible as a button and so separate actions. Maybe only add a short description above the buttons, something like "Extend your gallery by uploading new images or select files from your library.".

@jorgefilipecosta
Copy link
Member

I created an alternative PR that tries to follow the option 2 #12367 proposed in #9682 (comment). Any feedback is welcome.

@gziolo
Copy link
Member

gziolo commented Feb 1, 2019

@jorgefilipecosta - where are we with this PR. I see your alternative PR opened. Did we decide which PR should be closed?

@gziolo gziolo added the [Block] Gallery Affects the Gallery Block - used to display groups of images label Feb 1, 2019
@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 7, 2019
@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 16, 2019

I think one should go with either just text or just buttons, and not a mix.

Btw Lets have consistency between Gallery and Single Image screens.

This is from the Add single image screen:
screen shot 2019-02-16 at 18 59 44

With inspiration from Add Single Image screen here is a suggestion for the Gallery screen:
gallery-add-images

gallery-add-images-2

So keeping with consistency number 3 would be the closest.

jorgefilipecosta added a commit that referenced this pull request Mar 25, 2019
…12367)

Fixes: #8309
Supersedes: #9682

This PR adds a media library button in the upload new image are of the gallery block.
Trying to follow the design 2 proposed in #9682 (comment) by @kjellr and approved by @karmatosed.

A new functionality that allows the gallery to open in library/add frame instead of the edit frame was added to MediaUpload, so when the user pressed the media library button in the add zone the user goes directly to the add to library section when using the edit gallery button on the toolbar the user goes to the edit gallery section.

## Description
I added a gallery;
I added some images;
I selected the gallery;
I checked that a new media library button exists in the add to gallery zone.
If I click on it I go directly to a frame that allows the addition of images from the library to the gallery.

## How has this been tested?

## Screenshot
<img width="997" alt="screenshot 2018-11-27 at 15 35 14" src="https://user-images.githubusercontent.com/11271197/49093136-64176280-f25b-11e8-82de-ff44cee4ab47.png">
@jorgefilipecosta
Copy link
Member

Hi all thank you for working on this issue, more specifically @kevinbazira for submitting this PR that started an important discussion about what path we should follow. Given that #12367 was merged I think we can close this PR if anyone was further comments or ideas for improvement feel free to comment on the PR or open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Feature] Media Anything that impacts the experience of managing media Needs Decision Needs a decision to be actionable or relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.