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

Add Gallery Block #740

Merged
merged 19 commits into from
Jun 9, 2017
Merged

Add Gallery Block #740

merged 19 commits into from
Jun 9, 2017

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented May 9, 2017

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

@mkaz mkaz self-assigned this May 9, 2017
@mkaz mkaz added the [Status] In Progress Tracking issues with work in progress label May 9, 2017
@jasmussen
Copy link
Contributor

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.

@mkaz mkaz force-pushed the add/317-gallery-block branch from 2039eb9 to 62ef812 Compare May 10, 2017 17:26
@jasmussen
Copy link
Contributor

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.

🎉

@mkaz mkaz force-pushed the add/317-gallery-block branch from ce749ba to 91fec76 Compare May 11, 2017 18:03
@mkaz mkaz force-pushed the add/317-gallery-block branch 2 times, most recently from b7e9be4 to 0e25d20 Compare May 22, 2017 19:31
@mkaz
Copy link
Member Author

mkaz commented May 22, 2017

@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?

@westonruter
Copy link
Member

Cross reference core Gallery widget: xwp/wp-core-media-widgets#120

@jasmussen
Copy link
Contributor

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?

I tweaked/updated some mockups to explore this better.

Neutral:

gallery neutral

Selected:

gallery 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:

screen shot 2017-05-23 at 11 34 25

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.

@klihelp
Copy link

klihelp commented May 23, 2017

looks good:)

@mkaz
Copy link
Member Author

mkaz commented May 23, 2017

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

@jasmussen
Copy link
Contributor

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 none

Align full width:

gallery full width

@nylen
Copy link
Member

nylen commented May 26, 2017

After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against master, either because the build will fail upon merge, or because this PR can add some new tests using this structure (details here).

@mkaz mkaz force-pushed the add/317-gallery-block branch from 0786012 to 2787e14 Compare May 30, 2017 22:06
@mkaz
Copy link
Member Author

mkaz commented May 30, 2017

Hi @nylen - I've rebased and updated the block including the new text fixture.
I'm a little stuck trying to get it to generate the .json and .serialized files - could you help by taking a look.

 1) full post content fixture core-gallery:
     TypeError: Cannot read property 'children' of undefined

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 :-)

@nylen
Copy link
Member

nylen commented May 31, 2017

@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 core-gallery.html fixture does not have the format expected by the attributes matchers:

url: attr( 'data-url' ),
alt: attr( 'data-alt' ),

As a result, these attributes are passed through the block as { url: undefined, alt: undefined }, which is converted to {} when serialized to JSON. Here is the resulting JSON fixture file, which should contain details about the images instead:

[
    {
        "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 alt in the data-url and data-alt attributes at all. Why not just grab them straight from the img tag?

@mkaz
Copy link
Member Author

mkaz commented May 31, 2017

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

@mkaz mkaz added [Feature] Blocks Overall functionality of blocks and removed [Status] In Progress Tracking issues with work in progress labels May 31, 2017
@mkaz mkaz requested a review from nylen May 31, 2017 19:31
@mkaz mkaz force-pushed the add/317-gallery-block branch 2 times, most recently from 30e366b to d6d4ec0 Compare June 5, 2017 16:13
mkaz added 3 commits June 5, 2017 09:34
Creates initial gallery block, with stubbed out functionality.
Will create tickets for drag/drop, upload, multiple styles
@mkaz mkaz force-pushed the add/317-gallery-block branch from d6d4ec0 to c3cbce6 Compare June 5, 2017 16:50
@@ -0,0 +1,10 @@

export default class GalleryImage extends wp.element.Component {
Copy link
Member

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">
Copy link
Member

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">
Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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.

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 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 );
Copy link
Member

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?

Copy link
Member Author

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

@mkaz
Copy link
Member Author

mkaz commented Jun 7, 2017

Thanks @mtias updated with your suggestions and switched out to simple function instead of component 👍

@mtias
Copy link
Member

mtias commented Jun 8, 2017

Thanks! Let's get it in and iterate.

@mkaz mkaz merged commit 749c69f into master Jun 9, 2017
@mkaz mkaz deleted the add/317-gallery-block branch June 9, 2017 14:52
@jasmussen
Copy link
Contributor

🎉

@westonruter westonruter mentioned this pull request Jun 11, 2017
@StaggerLeee
Copy link

Would like to ask for one non-GUI option for Admins, filter. To be able to limit max number of Items per gallery ?
Actually two in one, to be able to limit max number of galleries per Page/Post.

What is your advice, how can we continue to call "filters" now in React framework ?

@mkaz
Copy link
Member Author

mkaz commented Aug 18, 2017

@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?

@StaggerLeee
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants