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

Gallery Block: Add Media Library button to the upload new image area #12367

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Nov 27, 2018

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

screenshot 2018-11-27 at 15 35 14

@simison
Copy link
Member

simison commented Nov 28, 2018

Could this section be its own independent component so that other blocks could re-use it?

It would be super useful when building e.g. alternative gallery blocks or slider blocks.

The component could even include the drop-zone component and support props supported by media-placeholder component. It would basically be generic "add additional media to this block" -wrapper.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Hey @jorgefilipecosta - this looks pretty close. Not sure if it's still WIP. I gave it a review anyway 😄

Would be cool to get a designer to have a look over it. I was thinking a bit on how the button styles were different to the other buttons and wondering if they could be brought closer to or based off of other existing styles instead of just the default.

<li className="blocks-gallery-item has-add-item-button">
<li className={ classnames(
'blocks-gallery-item',
'has-add-item-button',
Copy link
Contributor

Choose a reason for hiding this comment

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

has-add-item-button seems quite similar in intent to block-library-gallery-add-item. From what I could find has-add-item-button is in the wrong place (style.scss instead of editor.scss) and is superceded by your new class, so I think it can be removed:

// Make the "Add new Gallery item" button full-width (so it always appears
// below other items).
.blocks-gallery-item {
&.has-add-item-button {
width: 100%;
}
}

gallery
value={ images.map( ( img ) => img.id ) }
render={ ( { open } ) => (
<IconButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Did some testing, and the media library seems to fall out of sync quite easily:

  1. Create a new gallery with two images
  2. Click on the new media library button in the gallery appender
  3. Observe that the two images already added are not present in the list of images that can be added
  4. Close the media library without making any changes
  5. In the gallery block, use the 'x' icon button in the top right corner of one of the images to remove it.
  6. Click on the new media library button again
  7. Observe that the selection of images has not updated.

Also seeing some weird behaviour when clicking the media library button and switching to the 'Edit Gallery' view, it doesn't always seem to match what can be seen when editing the gallery from the edit toolbar button.

I know you have another PR open that seems related - #12435 - do you think that will solve it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @talldan, yes we have bugs in the media gallery that make it easily out of sync, I expect #12435 to address these sync issues.

const currentState = value ? 'gallery-edit' : 'gallery';
let currentState;
if ( galleryOpenInLibrary ) {
currentState = 'gallery-library';
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest how did you know about this setting, are there docs somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @talldan I did not find any documentation regarding this :( I checked what states we have by exploring the core code.

}
}

.block-library-gallery-add-item__upload-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that hovering over the upload button causes the media library button to move slightly to the right and the button text moves slightly downwards. Guessing that's caused by the border.

I also wondered if there's an existing button style that might provide a better base, like the 'tertiary' button.

@@ -78,6 +78,7 @@ class MediaUpload extends Component {
allowedTypes,
multiple = false,
gallery = false,
galleryOpenInLibrary = false,
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 convinced the prop name conveys the right meaning. I think renaming it to something like 'addToGallery' could be clearer.

@gziolo gziolo added [Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Enhancement A suggestion for improvement. labels Feb 1, 2019
@jasmussen
Copy link
Contributor

I would love for us to revisit, holistically, the appender overall, to give more options and a better overall design going into phase 2. However that is probably best done separately, and not as part of this bugfix. 👍 👍for this as a good iteration.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch from 70134dd to e4b97f5 Compare February 16, 2019 15:32
@jorgefilipecosta
Copy link
Member Author

Hi,
I'm sorry for the delay in answering I missed the notifications I think I probably unsubscribed to them by mistake.

Could this section be its own independent component so that other blocks could re-use it?

It would be super useful when building e.g. alternative gallery blocks or slider blocks.

The component could even include the drop-zone component and support props supported by media-placeholder component. It would basically be generic "add additional media to this block" -wrapper.

I agree, my plan is, for now, merge the enhancements in the gallery get them a little bit tested and then after everything is polished extract and expose a generic component.

Hi @talldan, thank you for your review, I think I addressed all your comments.

@kjellr
Copy link
Contributor

kjellr commented Feb 18, 2019

Hey @jorgefilipecosta, thanks for reviving this PR. This works well in my testing!

A couple small design notes:

screen shot 2019-02-18 at 8 57 14 am

  1. The button styling is a little odd — the text is blue, and it almost looks disabled to me? I'd expect the button to share the color/border/shadow of our other secondary buttons:

screen shot 2019-02-18 at 8 56 46 am

  1. There's a little extra margin below the dotted outline. It looks like it's inheriting the 16px bottom margin for gallery items, but that can be zeroed out so there's even spacing all around it.

@jorgefilipecosta
Copy link
Member Author

Thank you for the review @kjellr I will look into fixing these issues.

My plan was to after merging this PR extract a component that allows rendering the gallery appender. I noticed that the gallery appender being implemented here is just equivalent to MediaPlaceholder without label and icon, with the media library button opening the media library in a different state, and with some custom styles.
It may be possible refactor this changes to use MediaPlaceholder in the gallery appender by adding two additional optional props one that enables a custom style and one that allows opening the media library in a specific state (the add to gallery mode). I will check if this is, in fact, a possibility and update the PR accordingly.

@simison
Copy link
Member

simison commented Feb 20, 2019

Would it make sense if MediaPlaceholder could take a child component?

Or perhaps it should be called MediaContainer instead:

<MediaContainer>
  <Gallery />
</MediaContainer>

Where <Gallery> could be also <Slideshow> or <TiledGallery> for Jetpack or even something like <VideoPlaylist>. Child component would only care about files array and how to visualize it — the wrapper would take care of handling the files, dropzone, showing a placeholder when there are no files etc. When there are files, wrapper would show the placeholder under the gallery component.

Something like (very much simplified):

function MediaContainer(WrappedComponent) {
  return class extends Component {
    // handle state, file uploads, notices etc

    render() {
      if ( ! this.state.files.length ) {
        return <MediaPlaceholder />
      }

      return (
        <Fragment>
          { this.props.noticeUI }
          <DropZone />
          <WrappedComponent files={ this.state.files } { ...this.props } />
          <MediaPlaceholder title={ false } />
        </Fragment>
      );
    }
  };
}

export default withNotices( MediaContainer );

@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch 2 times, most recently from a6c7335 to d8b8f3b Compare February 20, 2019 17:22
@jorgefilipecosta
Copy link
Member Author

The code was updated, now instead of custom add item logic MediaPlaceholder handles everything (it got new additions). The gallery code and styles got simplified with this change.
cc: @sirreal, @simison

@kjellr The design was changed and I think your points were addressed:
screenshot 2019-02-20 at 17 03 46

Hi @simison,
Right now with this change the MediaPlaceholder already fits most use cases and some functions like the one that handles the addition of new files are not required anymore.
In the idea, you are proposing MediaContainer is responsible for rendering the MediaPlaceholder, the MediaPlaceholder contains many block specific parts (icon, labels, mapping of file/media objects to attributes, etc... This props would also need to be passed to MediaContainer so I'm not sure if the gain is significant.
I think we can try to enhance MediaPlaceholder and then we may think of ways to generalize more.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I really like the recent changes, the MediaPlaceholder component becomes much more useful 🙌


if ( ! hasUploadPermissions && ! onSelectURL ) {
instructions = __( 'To edit this block, you need permission to upload media.' );
}

if ( ! instructions || ! title ) {
if ( instructions === undefined || title === undefined ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is likely to break backwards compatibility, isn't it? Folks that have been relying on the default labels will no longer get them.

An alternative would be to accept null for these props so that folks can explicitly opt-out of the default label and instructions. It's much less likely folks are using a falsey value other than undefined to get the defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is likely to break backward compatibility, isn't it? Folks that have been relying on the default labels will no longer get them.

I think folks that relied on the defaults will still get them because in that case instructions or title are undefined.

What this allows is the ability to not render instructions or title by setting a null value or empty string in these fields.
There is minor behavior change previously if title/instructions were null or contained an empty string we applied default value now for this cases we don't. But I don't think someone would set the prop to a null/empty string and expect its value to contain the defaults.

onChange={ this.onUpload }
accept={ accept }
multiple={ multiple }
>
{ __( 'Upload' ) }
</FormFileUpload>
<MediaUpload
addToGallery={ addToGallery }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case when we'd disable addToGallery? I'd expect folks to always need to opt-in to this behavior when using the MediaPlaceholder.

It also seems to overlap with isAppender, although one is for styling and the other behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a use case when we'd disable addToGallery? I'd expect folks to always need to opt-in to this behavior when using the MediaPlaceholder.

When the gallery/value was empty previously we went to the create gallery flow instead of the add to gallery one. If we default to addToGallery we would be applying a behavior change and go by default to the add flow instead of the create flow.

It also seems to overlap with isAppender, although one is for styling and the other behavior.

Yes, I preferred to have a flag for behavior and another one for the style. So we allow more customization.

Thank you for the review @sirreal 👍

@jorgefilipecosta jorgefilipecosta added this to the 5.4 (Gutenberg) milestone Mar 14, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch from d3d51b7 to 71db241 Compare March 14, 2019 17:18
@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch 5 times, most recently from 479a80b to 75d9271 Compare March 15, 2019 08:33
@jorgefilipecosta
Copy link
Member Author

Hi @simison,
I remember that gallery had a design for the add button equivalent to the one you shared but it was meanwhile changed and the add button is already wide on master:
Screenshot 2019-03-15 at 08 50 22

I rebased the PR did a new set of test. I reverted the fix to hoover spacing problem -- the fix caused a bug when the gallery is in the placeholder state, and I noticed this problem is already present on master. I would prefer if we address the problem separately.

@jasmussen @kjellr any thoughts regarding the current state of this PR? Any task remaining in your opinion?

@kjellr
Copy link
Contributor

kjellr commented Mar 15, 2019

@jasmussen @kjellr any thoughts regarding the current state of this PR? Any task remaining in your opinion?

This is looking good. Just one remaining item in my opinion: We should set it so that a click anywhere on the drop zone triggers the upload modal.

I do feel like we should consider changing the dropzone area as described above #12367 (comment), but that'd come with some tradeoffs, so I think it's fine to punt it for now and revisit later. 👍

@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch from 75d9271 to ed4859c Compare March 21, 2019 10:18
@jorgefilipecosta
Copy link
Member Author

Hi @kjellr, this PR was updated and all the appender area is clickable.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

@jorgefilipecosta This is looking great! I pushed two small changes:

Screen Shot 2019-03-25 at 8 41 28 AM

This should be ready to go!

…; Media Upload: Allow galleries to open in the library frame.
@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch from 1fdb53f to 934a30c Compare March 25, 2019 15:35
@jorgefilipecosta jorgefilipecosta force-pushed the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch from 934a30c to 3eb3261 Compare March 25, 2019 16:14
@jorgefilipecosta jorgefilipecosta merged commit 3c1fe58 into master Mar 25, 2019
@jorgefilipecosta jorgefilipecosta deleted the Gallery-Block-Add-Media-Library-buttons-to-the-upload-new-image-area branch March 25, 2019 16:35
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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants