-
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
Add Gallery Block #740
Add Gallery Block #740
Conversation
Thrilled you're working on this! I tried building but it's erroring for me. But I see that it's in progress, let me know if/when you'd like eyes on this. |
2039eb9
to
62ef812
Compare
Nice first stab! Can you use the new Placeholder components instead of making the placeholder manually? See also embed.js for an example. I know the design is slightly different. It would be really good to hook up the media library, I think lots of pieces fall in place then. 🎉 |
ce749ba
to
91fec76
Compare
b7e9be4
to
0e25d20
Compare
@jasmussen Media Library integrated, check it out. Any suggestions on removing images already added or adding more? We could click the whole block and take back into Media Library? |
Cross reference core Gallery widget: xwp/wp-core-media-widgets#120 |
I tweaked/updated some mockups to explore this better. Neutral: Selected: To explain the blueprint — the toolbar that shows up on the block itself is the "Quick Toolbar". The "Edit" button on that toolbar, same as for the gallery block in WordPress currently, opens the media library on the "Edit Gallery" page, that looks like this: The sidebar on the right, is the "Inspector". The intricate details here are still a little bit in flux, and subject to change, so these are best handled in a separate PR. But essentially these contain more advanced aspects of editing the block, than can fit in the quick toolbar. |
looks good:) |
I understand for the Quick Toolbar to include the Edit which will go back to Media Library, and the drop down to pick the different styles. However, I'm not sure what the alignment (left, right, center, and stretch) means for a gallery |
So keep in mind we're building desktop publishing for the web. And so it makes sense to think of a gallery more as "a blob of images" than a traditional gallery, which is just a kitchen sink full of photos. For example, imagine a gallery with 3 pictures, set to 1 column in the options, floated left, with text flowing around it on the right. Or imagine a full-width/stretch'ed gallery set to 3 columns, as a separator between two paragraphs. The alignments are basically "layout tools", and users can do anything with them they like. Though ideally we'd be able to re-use the alignments from the image block 1:1 without much in the way of changing it. Some quick and dirty mockups... align none: Align full width: |
After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against |
0786012
to
2787e14
Compare
Hi @nylen - I've rebased and updated the block including the new text fixture.
I'm seeing the above error. Once the tests part get straightened out, the block should be ready to go - so can use a review too :-) |
@mkaz the issues with the test framework itself are addressed in the latest few commits. However, these tests are pointing to some problems with the block. The markup in the url: attr( 'data-url' ),
alt: attr( 'data-alt' ), As a result, these attributes are passed through the block as [
{
"uid": "_uid_0",
"blockType": "core/gallery",
"attributes": {
"images": [
{},
{}
]
}
}
] Previously, any object passing through that part of the test code was assumed to be a React component, which was causing the specific issue you were seeing. I've fixed that. Separately, I don't think we should duplicate the URL and |
Thanks @nylen - I thought I saw somewhere it was preferred to include the attributes in data, I'll switch to get them out of the img tag itself |
30e366b
to
d6d4ec0
Compare
Creates initial gallery block, with stubbed out functionality. Will create tickets for drag/drop, upload, multiple styles
Instead of reopening the Media Gallery, which the Media Upload Button does, when editing the gallery requires opening the Edit Gallery modal. Also requires setting selection and event which translates backbone data. Props to @georgeh, who I paired with developing See wp.media.editor from wp-includes/js/media-views.js
d6d4ec0
to
c3cbce6
Compare
@@ -0,0 +1,10 @@ | |||
|
|||
export default class GalleryImage extends wp.element.Component { |
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.
Dos this need to be an element.Component
or could it be just a function?
@@ -0,0 +1,10 @@ | |||
<!-- wp:core/gallery --> | |||
<div class="blocks-gallery"> | |||
<div class="blocks-gallery-image"> |
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.
These could be figure
instead of div
for the markup.
export default class GalleryImage extends wp.element.Component { | ||
render() { | ||
return ( | ||
<div key={ this.props.i } className="blocks-gallery-image"> |
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.
key
here isn't doing anything.
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 wasn't addressed.
<MediaUploadButton | ||
onSelect={ setMediaUrl } | ||
type="image" | ||
auto-open |
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.
Curious why we chose dash-separated here, instead of autoOpen
. Camel-case to be the convention with prop naming.
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 how it was specified in MediaUploadButton, I'd prefer to ship and then fix in another PR since it touches a couple blocks at once. Created issue #1083 to handle
} ); | ||
} | ||
|
||
editFrame.on( 'insert', updateFn ); |
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.
Do we need to detach these event handlers after the edit frame is closed, to avoid risk of zombie event listeners?
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.
On the frame Unmount the entire frame is destroyed, so the event handlers should be taken care of with that. See https://github.com/WordPress/gutenberg/blob/master/blocks/media-upload-button/index.js#L36
Thanks @mtias updated with your suggestions and switched out to simple function instead of component 👍 |
Thanks! Let's get it in and iterate. |
🎉 |
Would like to ask for one non-GUI option for Admins, filter. To be able to limit max number of Items per gallery ? What is your advice, how can we continue to call "filters" now in React framework ? |
@StaggerLeee Are you looking for an option to limit a user on how many items they can put into a gallery? Is this necessary? I think something like that could easily be circumnavigated by inserting two gallery blocks into the same post, or just inserting multiple images. Can you create a new issue for this discussion? |
Maybe in some later stage. Now you all have to much to do. I meant you moved everything to blocks, all widgets. When they are used on Homepage template could be nice to have some limit option, and still give User power to change things on Homepage. |
Initial PR for adding the gallery block - the initial code just stubs out the basics. Will need to add in multiple style galleries, drag-drop reordering, image upload, and any additional styles, will break these into additional smaller issues so it's not all done under one large PR.
See Issue #317