-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Added drag and drop upload to gallery block #3819
Conversation
dcaf9ab
to
f8eb43e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, I didn't have the time to test it yet, I'll approve once done.
blocks/library/gallery/block.js
Outdated
@@ -113,6 +115,21 @@ class GalleryBlock extends Component { | |||
} ); | |||
} | |||
|
|||
appendImageAttributes( currentImages, newImages ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Personal opinion but is this function really useful as a separate method? I generally try to minimise the component methods if not necessary (repeated code, exposed functions, event handlers)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is not useful as a separate method. Normally, I try to avoid implementing a function inside another function but in this case and given that it is a minor function I think it is ok. So I followed your advice 👍
blocks/library/gallery/block.js
Outdated
@@ -167,6 +190,7 @@ class GalleryBlock extends Component { | |||
return [ | |||
controls, | |||
inspectorControls, | |||
dropZone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropzone is a component that's dependent visually on its parent. And in the image block it's included inside the "placeholder" instead of using the parent element of this component (which is unknown really from a block author's point of view).
Should we move this inside the placeholder and the gallery container instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved I added first as a test, and then as things were working great I kept it there, but you are totally right it is preferable to add it inside a container.
f8eb43e
to
bb68a6c
Compare
Thank you for having a look @youknowriad the code was updated :) |
a61fb03
to
e1e5446
Compare
… new items without going to media through the media library.
e1e5446
to
dd9281a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great. Thanks
This change address part of the concerns raised on #1704. It provides a way to add new images to a gallery without going to media through the media library.
How Has This Been Tested?
Create a gallery block drag&drop some images to it, see the images were added to the gallery. Add more images and see it is possible to use this mechanism even in galleries with content.
See existing gallery functionality still works as expected e.g: remove images/ change images.
Screenshots (jpeg or gifs if applicable):