-
Notifications
You must be signed in to change notification settings - Fork 798
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
Feature: add support for a Tiled Gallery layout (Square Tiles) as a Gutenberg block #9434
Feature: add support for a Tiled Gallery layout (Square Tiles) as a Gutenberg block #9434
Conversation
Related: #8527 |
03af8e0
to
98e3faa
Compare
Looks like the CI build started failing when I changed some comments? Is there something else going on here? (I'm pretty certain it passed originally.) Can that be easily rerun? Aha! Here's the passing test, looks to be on the same changeset as the failing test? |
I reran the 5.3 tests which had failed. All green now 👌 |
Given that this PR comes from a fork of the Jetpack repo, I felt the need to drop the instructions on how to achieve a local checkout of this branch for those working with a direct clone of Automattic/Jetpack. To check this PR:
Make sure Gutenberg is installed and Jetpack is connected. |
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 a great start! 👍
I've encountered some of the issues that you've already mentioned in the PR description, seems they'll need some additional work, but they can be taken care of when it's certain that the PR is heading in the right direction.
A fundamental question is how Jetpack-powered gallery features should be made available to users – as an extension to core/gallery, or as a separate block (or some other approach). This PR is a demonstration of how this can work well as a separate block.
Looking at the code, I see a lot of duplication from the original gallery block. I think we can attempt to reuse much of the original gallery block code, and that would save us a lot of repetition, but also would prevent us from having to update this block every time a small change is made to the original gallery block. After all, the original gallery block will probably stay around for a while, so I think it will be a good idea if we follow it closely with any Jetpack gallery blocks. This way they could benefit from any future changes/improvements to the core gallery block. For example, I like how the original gallery block resizes the images to fit the available space:
Have you experimented with that approach already?
Only one tiling algorithm supported at present. If we think this approach is sound, can look at porting other tiling options as follow-up PRs.
Do you have any thoughts about where this would be selected? Should each of these be a separate block, or could it be an inspector control? I think it would be very intuitive if we have all these as an inspector control setting and users are able to change them as they can in the media popup for the original gallery shortcode.
Longer term it would make sense for this behaviour to be integrated into the react components.
👍 Perhaps something that all gallery blocks could reuse?
Images render squished when previewing in Gutenberg.
This is easier to reproduce and observe with images that have a larger ratio. We should test with a variety of sizes and ratios to make sure all of them are rendered properly and don't appear squished.
A hard-coded value is used for content_width. I suspect this means that tiled galleries will not slot in beautifully in some themes. This value is not currently available to the js/front end (that I'm aware of). Is this heavily used?
In my previous theme development experience, I used that often. It is pretty convenient, and we should take advantage of it. Exposing the value to the client is pretty straightforward, so you could do it to avoid hardcoding the content width.
The jsx code for this plugin is built as a new entry point in webpack. This works fine for now, but long term it would be much better to bundle all of Jetpack's Gutenberg jsx as a single file.
👍 This could be done eventually when more blocks are merged into the Jetpack repo.
Again, this is great work 👍 I'm looking forward to seeing some more 💯
modules/tiled-gallery/block-edit.jsx
Outdated
<ImagePlaceholder key="gallery-placeholder" | ||
className={ className } | ||
icon="format-gallery" | ||
label={ __( 'Gallery' ) } |
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.
What do you think about labelling it "Jetpack Gallery" instead, so it is different from the default Gallery block?
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.
Good point! I wish I'd spotted that earlier!
public static function enqueue_block_editor_assets() { | ||
wp_register_script( | ||
'jetpack-tiled-gallery-block', | ||
plugins_url( '../../_inc/build/modules-tiled-gallery-block.js', __FILE__ ), // this is built as a new webpack entry point |
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.
Instead of __FILE__
here we could use JETPACK__PLUGIN_FILE
to avoid the ../../
relative paths.
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.
That's more robust, thanks. Now fixed :)
…older), transform to/from core/gallery
…nclear on how to store attributes/state so they are available to PHP render)
… front end, so image state persists (and renders)
…g on editor reload due to assuming wrapper element class name based on core/gallery block (was causing validation error) – fixed by simplifying selector
- reset line-height - zero out figure margin
…ery to js work-in-progress)
…served (fix no data/validation error)
… we can use them for smart tiling layouts implemented in js)
…ur block-based tiled galleries (short term hack)
…we don't use (that we stole from core/gallery..)
…f user navigating out of editor when clicking the gallery
…umn-count layout experiment)
… (for now - ideally we'd have a jetpack-branded icon or a "tiled" icon)
…utenberg script asset (using JETPACK__PLUGIN_FILE instead of current-file-relative __FILE__)
…holder component for consistency and to disambiguate from core/gallery
98e3faa
to
e8dd3eb
Compare
@haszari could you create a version of this branch but on this same repo instead of being a branch on a fork ? |
@oskosk Will do, thanks for following up. It will be interesting to see how stale this code is – hasn't been rebased in a while. Will let you know when it's up. |
I"m considering playing with this as a testbed for the Gutenberg/Calypso integration project, so I might as well do it. |
Caution: This PR has changes that must be merged to WordPress.com |
Caution: This PR has changes that must be merged to WordPress.com |
Caution: This PR has changes that must be merged to WordPress.com |
Closing in favour of #9903 |
Based on Haszari's work in: Automattic/jetpack#9903 & Automattic/jetpack#9434 Contains some modifications to make this work with latest Gutenberg and with Calypso's linters. Co-authored-by: haszari <haszari@cartoonbeats.com>
Based on Haszari's work in: Automattic/jetpack#9903 & Automattic/jetpack#9434 Contains some modifications to make this work with latest Gutenberg and with Calypso's linters. Co-authored-by: haszari <haszari@cartoonbeats.com>
Add tiled gallery Gutenberg block Based on @haszari's work in: Automattic/jetpack#9903 & Automattic/jetpack#9434 Contains some modifications on top of their work to make this function with Gutenberg v3.3 and with Calypso's linters. Co-authored-by: haszari <haszari@cartoonbeats.com>
Related: #8527 (see limitations below)
Changes proposed in this Pull Request:
Currently, there's no way to use Jetpack Tiled Galleries in Gutenberg. This pull request implements a new Gutenberg block which provides one of the Tiled Gallery layouts as a Gutenberg block. This block can be easily transformed to and from the core Gutenberg gallery block.
This is a proof-of-concept which demonstrates one way to provide Tiled Gallery features in Jetpack. There's lots more work to do!
A fundamental question is how Jetpack-powered gallery features should be made available to users – as an extension to
core/gallery
, or as a separate block (or some other approach). This PR is a demonstration of how this can work well as a separate block.Testing instructions:
I have a sandbox running a production build of this branch - ping me if you want an account to test.
https://haszari.wpsandbox.me
Otherwise, to test this on your WordPress site....
You can add Jetpack Gallery blocks using the Gutenberg switcher. The interface is somewhat similar to the core block experience, with live preview of the Square Tiles layout, and using the (legacy) modal for adding or editing the images and captions. There are settings for number of columns and what the images should link to (attachment, file, or no link).
Using the block switcher you can transform any core gallery to a tiled gallery, and vice versa.
Limitations/Notes
core/gallery
implementation, especially for the edit component. This delegates image selection, ordering etc to the legacy edit gallery modal. Ideally the uploading and reordering of images (and other core editing) would be implemented within Gutenberg (WYSIWYG).core/gallery
, which has implemented direct manipulation).core/gallery
).content_width
. I suspect this means that tiled galleries will not slot in beautifully in some themes. This value is not currently available to the js/front end (that I'm aware of). Is this heavily used? If so, we can look at ways to make it available to the client side.