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

Gutenberg: Tiled gallery block #27458

Merged
merged 15 commits into from
Nov 29, 2018
Merged

Gutenberg: Tiled gallery block #27458

merged 15 commits into from
Nov 29, 2018

Conversation

simison
Copy link
Member

@simison simison commented Sep 26, 2018

Rewrite of the Tiled gallery block.

Apologies for a huge PR. πŸ™‡β€β™‚οΈ This is quite a beast of a block! Complete rewrites are a bit tricky to do in portions. Probably good if you review files without the diff.

(I updated the description here so comments below might seem a bit out of context)

Screenshots

screen shot 2018-10-23 at 16 03 27

screen shot 2018-10-23 at 16 02 16

screen shot 2018-10-23 at 16 01 44

Testing & features

  • Spin up Jurassic Ninja testing site
  • Alternatively if you use local Jetpack setup, you can build the Jetpack preset:
    npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- --output-dir=/path/to/jetpack/_inc/blocks
    
    β€” Just remember to npm install first since this adds new deps.
  • Connect Jetpack
  • Create a new post
  • Insert Tiled Gallery block

Confirm that these work as expected:

  • You can resize the browser window, toggle Gutenberg editor sidebar, change the mobile browser orientation and the gallery will adapt to new size both at the theme and in the editor
  • You can switch styles from the toolbar
    • Mosaic and columns layouts are missing; they just render squares -layout
  • You can write captions for each image: circle layout doesn't support captions, though.
  • You can remove images from small "x" or pressing backspace while the image is selected
  • Different block-alignments and block-widths; make sure to test with a theme that supports them, such as Suki or Twentynineteen
  • You can add and remove images using buttons as well via drag'n'drop
  • You can save the post, refresh the page, and tiled gallery block still loads
  • Saved tiled galleries with different layouts, columns and other settings as well with captions render at the theme side without issues
  • You can change a number of columns from the sidebar and see the layout change: only squares and circles support "columns"; for other layouts, columns-setting is disabled.
  • You can transform tiled gallery to/from core-gallery block and to image blocks.
  • Enable lazy loading images & Photon CDN from Jetpack settings and confirm that the gallery plays nicely with those features

Known issues

  • Left+right block-alignments have some silly bug
  • Previews in style picker look wonky but probably acceptable for v1
  • Block does not transform from [gallery] block-code and I'm not sure why. Might be colliding with core-gallery since these share the same shortcode.

Technical specs

Requires Jetpack side PR: Automattic/jetpack#10375

  • I copied over Core gallery from Gutenberg as a starting point
    • There are some areas of code that tiled gallery could share with core-gallery, but I haven't done much about it yet. I've kept gallery-image.js exactly as it's Gutenberg so that we could use it as a component from Gutenberg later on. Also getActiveStyle() helper is directly from Gutenberg.
    • From the previous trial version of tiled-gallery, I basically kept just the math for square tiles layout, simplifying the object that the math-function returns.
  • Square tiles don't really need "rows" HTML nodes, but I've kept them here for simplicity since mosaic and columns layouts depend on them.
  • Uses ResizeObserver to fire events when the gallery's wrapper element has size changes. Changes could be caused by mobile screen orientation changes, changes in the browser window, by appearing elements such as Gutenberg sidebar etc.
    • Only Chrome supports it, so I'm using a polyfill.
    • ResizeObserver API is really simple and it's performance is great because it doesn't need to be polling for size changes or fire on every window.onResize. Read more from Google and or check out the spec.
    • We can ignore all the usual viewport events these implementations typically listen to, as well all the events changing the editor size in Gutenberg.
    • Resize events are throttled by 200ms both at the theme and editor, no matter how many editor blocks we need to redraw.
  • Theme side view.js does not rely on React. I purposefully choose to implement it as vanilla JS to keep payload smaller, albeit causing a bit more maintenance for us. I explored rendering with React as well so if we decide so, it would be quick enough change to that implementation instead.

Missing two layouts

So Mosaic and Columns is missing. Squares and circles are in.

We could decide to:

  • Port math over from PHP β€” I've been looking into this but quite badly blocked by it.
  • Use external JS library to calculate same or similar layouts.
  • Use Gutenberg's ServerSideRender component to render the two missing layouts via shortcode, as a temporary solution.

@simison simison added the [Goal] Gutenberg Working towards full integration with Gutenberg label Sep 26, 2018
@simison simison self-assigned this Sep 26, 2018
@matticbot
Copy link
Contributor

@mtias
Copy link
Member

mtias commented Sep 26, 2018

I'd definitely go for style picker for the variations β€” that's what it's meant for. It ensures it's accessible even with sidebar collapsed.

@MichaelArestad
Copy link
Contributor

Random order - The tiled gallery has "random" image order option, which will shuffle images to different order on each page refresh. Should we include that feature here?

It depends. If the order of images allows the user to rearrange them, then we should probably not randomize them. However, if in any of the layouts, we can't allow dragging to rearrange, then random order would be good.

Captions - Should we include the animation in the block? I'd vote to drop that and do what ever core gallery does.

I agree with you. Let's drop those in favor of the way core gallery handles captions.

Style picker

Let's definitely go with the style picker for variations. I think they're better at conveying meaning than a 20x20 icon and, as Matias mentioned, it's what they're designed for.

Circle layout issues

I think we should likely disable captions on the circles altogether. I'm not sure we want to mess with the blue border, though. @mtias what do you think?

Margin between the tiles is 2px. Should user be able to adjust that?

How easy is that to do? Is the mosaic script fast enough for that to work well?

Icon

This was from the earlier rounds of design, but might work.

@mtias
Copy link
Member

mtias commented Sep 27, 2018

I think we should likely disable captions on the circles altogether.

Seems sensible to me.

@mtias
Copy link
Member

mtias commented Sep 27, 2018

By the way, there's an idea about duplicating block styles in a panel on the sidebar that would automatically make these available in both places, which I think would be great.

@simison simison force-pushed the update/tiled-gallery-updates branch from bd64ef2 to ae54a1b Compare October 11, 2018 05:59
@MichaelArestad
Copy link
Contributor

I tried testing this today, but couldn't get it to show up in Jurassic Ninja (I did connect Jetpack). Ideas?

@simison simison force-pushed the update/tiled-gallery-updates branch from 5fbd0ff to 9c78e41 Compare October 12, 2018 07:20
@simison simison requested a review from a team October 12, 2018 08:00
@simison simison added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 12, 2018
@simison simison force-pushed the update/tiled-gallery-updates branch 2 times, most recently from df45582 to 348851c Compare October 12, 2018 09:05
@simison
Copy link
Member Author

simison commented Oct 12, 2018

@MichaelArestad

I tried testing this today, but couldn't get it to show up in Jurassic Ninja (I did connect Jetpack). Ideas?

Looks like the JN link was a bit outdated (this now requires a branch from Jetpack, too). I updated and tested it works now. πŸ‘ Thanks for the heads up.

I fixed a couple smaller bugs with circle -layout, rebased+squashed commits so this would be good for code review now. This is not ready block but I'd like to leave following work to different PRs to make reviewing them easier & more focused.

@simison simison removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 15, 2018
@simison
Copy link
Member Author

simison commented Oct 15, 2018

πŸ’₯ I found some issues with wp_kses and <style> so I need to make some refactoring before this is good for review again. 😞

@simison simison force-pushed the update/tiled-gallery-updates branch from 348851c to 92b170b Compare October 23, 2018 10:42
@simison simison added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 23, 2018
@simison simison changed the title Gutenberg: update Tiled gallery block Gutenberg: Tiled gallery block Oct 23, 2018
@simison simison requested a review from a team October 23, 2018 14:04
@ghost
Copy link

ghost commented Nov 29, 2018

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Calypso maintainers do their job. If you think 'Testing instructions' are not needed for your PR - please explain why you think so. Thanks for cooperation πŸ€–

Generated by 🚫 dangerJS

@simison
Copy link
Member Author

simison commented Nov 29, 2018

Was the shrinkwrap rebuilt recently? I noticed there have been several rebases and force pushes.

Good call. I just rebased to confirm that shrinkwrap file is still up to date.

@simison simison force-pushed the update/tiled-gallery-updates branch from e3f65a7 to 3407901 Compare November 29, 2018 13:11
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.

This shouldn't be included anywhere now. Let's merge and iterate πŸ™Œ

@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2018
@simison simison added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 29, 2018
@sirreal
Copy link
Member

sirreal commented Nov 29, 2018

Tests are all green now even though GH doesn't reflect that. Reporting mechanism hasn't been working well recently.

@lezama
Copy link
Contributor

lezama commented Nov 29, 2018

🚒 IT πŸŽ‰

@simison simison merged commit 3a151cc into master Nov 29, 2018
@simison simison deleted the update/tiled-gallery-updates branch November 29, 2018 13:55
jeherve pushed a commit to Automattic/jetpack that referenced this pull request Nov 29, 2018
Load Tiled Gallery Gutenberg block assets.

## Testing

- Apply the commit 9e27e59 to fix loading block assets at appropiate loading order. (PR #10739)
- In Calypso, switch to branch `update/tiled-gallery-updates` (PR Automattic/wp-calypso#27458) and build the blocks to get `_inc/blocks/tiled-gallery/view*` files:
    ```
    npm run sdk -- gutenberg client/gutenberg/extensions/presets/jetpack/ \
        --output-dir=~/jetpack/_inc/blocks/
    ```
- Insert tiled gallery block in the editor, add some awesome images and publish the post
- Check the post frontend, you should observe `tiled-gallery/view.*` files load and the gallery shouldn't look too broken. It might look just a tad bit broken because it's work in progress. ;-)
* Throttled resize event
* Compares gallery width to its previous width to decide if to resize it.
*/
const throttleOnResize = throttle( entries => {
Copy link
Member Author

@simison simison Nov 30, 2018

Choose a reason for hiding this comment

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

I'm wondering if we would get better performance by using debounce and how that would look visually when resizing the window.

Differences: https://css-tricks.com/debouncing-throttling-explained-examples/

Also if we don't have to import this from lodash, we could ditch one required dependency since in WordPress we would just enqueue the whole lodash which is kinda wasteful. Another consideration is to change how SDK externalizes that library; perhaps only for editor.js?

Tracked in #28933 and #28978

gititon pushed a commit to Automattic/jetpack that referenced this pull request Dec 7, 2018
Load Tiled Gallery Gutenberg block assets.

## Testing

- Apply the commit 9e27e59 to fix loading block assets at appropiate loading order. (PR #10739)
- In Calypso, switch to branch `update/tiled-gallery-updates` (PR Automattic/wp-calypso#27458) and build the blocks to get `_inc/blocks/tiled-gallery/view*` files:
    ```
    npm run sdk -- gutenberg client/gutenberg/extensions/presets/jetpack/ \
        --output-dir=~/jetpack/_inc/blocks/
    ```
- Insert tiled gallery block in the editor, add some awesome images and publish the post
- Check the post frontend, you should observe `tiled-gallery/view.*` files load and the gallery shouldn't look too broken. It might look just a tad bit broken because it's work in progress. ;-)
@gsallman
Copy link

Captions - Should we include the animation in the block? I'd vote to drop that and do what ever core gallery does.

I agree with you. Let's drop those in favor of the way core gallery handles captions.

I am but a humble user putting in my opinion (with zero experience on Github, so apologies for any faux pas). If you are talking about how captions are rendered on the delivered page, then no, please do not do what the core gallery does. But if you are only talking about within the block editor, that's fine.

The way the Core gallery handles the display of captions on the rendered page is unusable. (I don't know how it got past testing). I've tried it on several sites, and it is a responsive nightmare. In many cases the captions completely cover the images. Generally, I want visitors to be able to see all of the image, not have large portions of it obscured by text. But, I do want the caption text to be available for the curious (and search engines). On the other hand, the way the captions are currently handled in Jetpack with an animation is perfect, and shouldn't be changed. (Sorry for butting in here).

@simison
Copy link
Member Author

simison commented Dec 11, 2018

Thanks for the feedback, @gsallman. It's helpful.

Responsiveness of the Tiled gallery block is definitely on top of our priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tiled Gallery [Goal] Gutenberg Working towards full integration with Gutenberg Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants