Add Image Compare block

[mkaz]

Adds the Image Compare block as an experimental block, the image compare blocks allows you to place two images side-by-side (or above/below) and use a slider to reveal different aspects of them.

Here's an example:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Internal reference: p1HpG7-8NH-p2

Testing instructions:

• This is a beta block, so you will need to set in wp-config.php
  define( 'JETPACK_BETA_BLOCKS' true );

• Create new post . Add block to post and select before/after images

• The comparison tool should show within the editor, as it will look on front-end

• Confirm resizing editor the images resize as expected

• Confirm shows on front-end and works as expected

• Test various cases of missing images or unexpected usage

Proposed changelog entry for your changes:

• Adds new Image Compare block

Future Tracking Issues;

• What to show for AMP pages that can't load library? (See: Image Compare Block: Add AMP support #15653)
• Fix useResizeObserver to support older (current) WordPress versions without the Gutenberg plugin (See: Sharing: Add translator comments #15655)

Licensing

NOTE: This block uses code originally from the Knight Labs Juxtapose block, I have discussed with @pesieminski and he has cleared the license notice in the readme.md and at the top of the view.js files.

Files here:
https://github.com/Automattic/jetpack/pull/15598/files#diff-6bee5a8a6f949abff285c30df8232b6e
https://github.com/Automattic/jetpack/pull/15598/files#diff-84156f4fb84990fe9b2aba4b50eef7a5

[jetpackbot]

	Warnings
⚠️	pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against bd9ee53

[jasmussen]

Wonderful, Marcus! Excited about this block.

What to show for AMP pages that can't load library?

This is a common problem with blocks that feature interactivity, and as I noted in this ticket I created, I would love for there to be a generic fallback for blocks that aren't able to render in AMP contexts.

Specifically, just providing a button that says "View on [sitename]" feels like a fine solution, not just generically but for this block as well.

So the bigger questions are of a technical nature:

1. Are we able to detect the AMP context?
2. If yes, are we able to detect e-mail or other lo-fi contexts?
3. If not, perhaps we simply need to output this button if the JavaScript doesn't load.

If we were able to answer "yes" to 1 and 2, not only should we consider submitting this AMP & email fallback solution to the block editor project itself so all complex blocks could benefit from it, but we would be able to build in the button that says "View on [site]" and have confidence that clicking the button would work.

The problem with 3 is that it would also invoke in contexts where the user has disabled JavaScript, and begs the question whether we therefore also need to output text that suggests JavaScript is necessary.

Still, 3 seems the most straight-forward, an in absence of 1 and 2, we could do this:

<noscript>
<div class="wp-block-button">
	<a class="wp-block-button__link" href="[url]">View on [sitename]</a>
</div>
</noscript>

I'd tend to lean towards not needing the JS warning. Virtually everything requires JS these days, so if you actively disable it, you'll likely have an idea why things might not be working.

[jeherve]

Can't wait to have this in Jetpack, this block is so nice!

Below, a few drive-by comments.

When switching from side by side to above and below, should the placeholder be updated accordingly? It gets updated when images have already been added, but not before:

---

Travis currently has some errors that will stop us from merging this:

/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/edit.js
  38:36  warning  React Hook "useResizeObserver" is called in function "edit" which is neither a React function component or a custom React Hook function  react-hooks/rules-of-hooks
  40:2   warning  React Hook "useDebounce" is called in function "edit" which is neither a React function component or a custom React Hook function        react-hooks/rules-of-hooks
/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/icon.js
  4:1  error  Missing external, internal dependencies docblocks  wpcalypso/import-docblock
/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/save.js
  4:1  error  Missing external, internal dependencies docblocks  wpcalypso/import-docblock
/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/upload-placeholder.js
  69:3  error  Do not use setState in componentDidMount   react/no-did-mount-set-state
  74:4  error  Do not use setState in componentDidUpdate  react/no-did-update-set-state
/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/use-debounce.js
   4:1  error    Missing external, internal dependencies docblocks                                                                                                                                                                                              wpcalypso/import-docblock
  10:5  warning  React Hook useEffect has missing dependencies: 'callback' and 'delay'. Either include them or remove the dependency array. If 'callback' changes too often, find the parent component that defines it and wrap that definition in useCallback  react-hooks/exhaustive-deps
/home/travis/build/Automattic/jetpack/extensions/blocks/image-compare/view.js
  543:9  warning  'slider' is assigned a value but never used  no-unused-vars

[jasmussen]

Great thoughts, Jeremy, I'll respond to the conceptual and design aspects and let Marcus help with the difficult stuff (thanks Marcus).

When switching from side by side to above and below, should the placeholder be updated accordingly?

Not a bad idea.

It's not trivial to do, though, and the Placeholder component is a little in flux at the moment. Would you mind if I file this for a follow-up?

What if we used for now (reference)? It doesn't have the same fancy effect, but it would display the 2 images next to each other.

I'm actually concerned about providing an experience that isn't identical to what the user asked for. The fanciness of this block is exactly that it stacks the two images on top of each other and let's you interactively scrub the separator to see the difference. This is what you see in the editor and anything else has the potential to ruin the effect.

Imagine someone using this for a fun little "drag to reveal spoiler" and amp then reveals it ahead of time?

Or, more likely, the user is likely to write in the prose: "compare the images below, drag the slider to reveal", which will obviously be misleading in an amp context.

For that reason, I'd prefer a "view on site" button.

[jeherve]

That's a good point, whatever workaround we'd pick, it'd be quite far from the original behaviour of the block.

I don't know if we can implement a "view on site" button though; while that could be an option with sites using AMP's transitional mode, I don't think it would be possible on sites using AMP's standard mode, where the site is AMP-first.

cc @westonruter; would you have some ideas?

[jasmussen]

I don't know if we can implement a "view on site" button though; while that could be an option with sites using AMP's transitional mode, I don't think it would be possible on sites using AMP's standard mode, where the site is AMP-first.

What does CNN do for their PDF reader which doesn't work on AMP? It's part of the screenshot shown in WordPress/gutenberg#17764

[jeherve]

What does CNN do for their PDF reader which doesn't work on AMP?

I'll let @westonruter weigh in on that, but I believe that's only an option if your site has two different versions available: AMP pages and your native site. When using AMP's standard mode in the WordPress plugin, your native site already uses AMP. Here is an example:
https://medinathoughts.com/2019/04/30/amp-in-wordpress-the-wordpress-way/
The page above is served via AMP out of the box, there is no alternative URL.

[jasmussen]

Understood — but doesn't AMP entirely strip out external JavaScript, making it possible to serve the button using noscript tags? I will obviously defer to the experts here, as I haven't brushed up my AMP skills lately.

[westonruter]

Note that AMP has an amp-image-slider component for this very use case. So there should be no need to gracefully degrade here.

The only thing the AMP component isn't supporting yet is the ability to compare vertically. There is an issue for this but it just needs to get bumped I think: ampproject/amphtml#18634.

Until that is resolved, one option could be to hide that orientation toggle when the AMP plugin is active.

[nainar]

What kind of timeline would you need support for vertical image slider to be implemented in AMP?

[mkaz]

What kind of timeline would you need support for vertical image slider to be implemented in AMP?

@nainar Ideally would be in the next couple weeks, our target is the June Jetpack release, with feature freeze end of May — but if it takes longer we might be able to code around it as Weston suggested, or just not have that feature available in AMP until it's ready. Thanks!

[jasmussen]

That's a good point, if the AMP image slider does not support vertical comparison, that definitely enables users to get into situations where what they publish in AMP format is not what they expected.

A button, "View on [sitename]" would be a somewhat dull fallback for that, but at least it would work in the interim.

Alternately we can accept that it doesn't work for the time being, and revisit when it does?

A third option could be to detect the presence of the AMP plugin and simply disable the option for vertical comparison.

I'd personally be fine with either of the above approaches, but I would lean towards the simplest solution that gets us shipping. I would rather we ship a small thing and learn from it, than assume that this block will set the world on fire and optimize it to perfection only to learn it gets few downloads.

[jasmussen]

Just spoke with Jeremy about some particular use cases of AMP, and was impressed to learn that an entire website could be AMP-only!

In such situations, of course, there wouldn’t be anywhere but that website to see the contents of the block, so a “view on [sitename]” button as referenced from the CNN example does not make sense.

This leaves us a couple of options to move forward with:

1. Wait for vertical comparison to land in the AMP slider, and leverage that here.
2. Hide the option to choose vertical comparison in AMP contexts, until it lands.
3. Move forward without AMP support for the time being, and add it back when possible.
   What do you think?

We have a month to go before a Jetpack code freeze, so we might be able to leverage 1. But if not, both 2 and three seem like good options to keep the momentum going.

In any case, it doesn't all have to happen in one PR, so we might want to land this PR without amp support, and then look at followups.

[mkaz]

@jasmussen I'm going to aim for (2) that is if I can detect the plugin then hide the vertical option.

[westonruter]

I'm going to aim for (2) that is if I can detect the plugin then hide the vertical option.

@mkaz An easy way to detect is to check if the is_amp_endpoint() function exists (in PHP).

[jeherve]

An easy way to detect is to check if the is_amp_endpoint() function exists.

We have a helper for that, it may be useful:
class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request()

You can check how other blocks use it here, for example:

jetpack/extensions/blocks/google-calendar/google-calendar.php

Lines 52 to 66 in 70dab92

	if ( class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request() ) {
	return sprintf(
	'<div class="%1$s"><amp-iframe src="%2$s" frameborder="0" style="border:0" scrolling="no" height="%3$d" sandbox="allow-scripts allow-same-origin" layout="responsive"></amp-iframe></div>',
	esc_attr( $classes ),
	esc_url( $url ),
	absint( $height )
	);
	} else {
	return sprintf(
	'<div class="%1$s"><iframe src="%2$s" frameborder="0" style="border:0" scrolling="no" height="%3$d"></iframe></div>',
	esc_attr( $classes ),
	esc_url( $url ),
	absint( $height )
	);
	}

[westonruter]

We have a helper for that, it may be useful:
class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request()

That's not quite what is needed here. The question is not whether it is an AMP response but whether the AMP plugin is installed and active at all. If it is, then the vertical option should be hidden since it's not quite available in AMP yet.

[mkaz]

In testing, I noticed another issue was using images of different sizes or aspect ratios. There was an aggressive check in the library that would bail if they weren't the same. I removed this check since, it all still works, though mixing landscape and portrait gives interesting results, but no reason to break because of it.

[jasmussen]

Happy to report that the block works for me now:

The issue was almost certainly the check for dimensions. I was a little haphazard in my testing and used somewhat random things. But agree it's good to remove those checks.

Will push a little CSS now. Thanks Marcus!

[jasmussen]

Okay, I pushed some polish. There was some cruft to remove, some placeholders to adjust, some typos to fix, and some CSS to polish up. I think I caught all of it, and it now feels ready for final review again:

[jeherve]

This worked in my tests this time. I think this is a good first iteration as it is, we could merge this and iterate in future PRs!

There are a few things I noted while testing, and that I think would need to be addressed before launch:

---

The block doesn't look great when you don't use the Gutenberg plugin

---

As we discussed earlier, I find the lack of progress indicators disturbing. It's hard to know that the block is really working when you upload images.

---

Once you've uploaded images, you cannot edit / replace one or both of them.

---

As I mentioned in my last review, I think it'd be nice if we could leverage our existing image tag creation tools; it would give us more data per image, such as srcset.

---

I think it would be nice if the block supported wide and full sizes, since that can be handy for images we want to display.

[mkaz]

As we discussed earlier, I find the lack of progress indicators disturbing. It's hard to know that the block is really working when you upload images.

Agree, I think this can be a nice feature to add. Right now it matches the core "Media & Text" block, the current way to add an indicator is a bit complex. I plan to write up a ticket for improving media upload experience.

Once you've uploaded images, you cannot edit / replace one or both of them.

This is known, initially this was considered an added layer of user interface complexity and that it would be easier to just delete the block and start over then add in a complex click sequence to select to replace and not interfere with the slider. However, we might be able to do something with the new Replace Image toolbar and G2 interfaces. @jasmussen thoughts?

As I mentioned in my last review, I think it'd be nice if we could leverage our existing image tag creation tools; it would give us more data per image, such as srcset.

We can test some things out, but the library reads the main src attribute, so not sure it would benefit having a srcset of images.

I think it would be nice if the block supported wide and full sizes, since that can be handy for images we want to display.

Good suggestion, we should add this feature.

[jasmussen]

Such good reviews, such good conversation. You're all wonderful.

As we discussed earlier, I find the lack of progress indicators disturbing. It's hard to know that the block is really working when you upload images.

Agreed — for V2. But we might even want to look at making that PR to the core project, if we can somehow benefit Media & Text, then perhaps we would not have to write one-off code for this block, and benefit both.

Once you've uploaded images, you cannot edit / replace one or both of them.

Agreed — for V2.

We can test some things out, but the library reads the main src attribute, so not sure it would benefit having a srcset of images.

Seems fine to explore, but I'd go at least V2, possibly even V3.

I think it would be nice if the block supported wide and full sizes, since that can be handy for images we want to display.

Also agreed, but also V2, or at the very least a followup PR. This one may seem trivial, but I have that tingling sensation that it is absolutely not at all. Already now we had some challenges with the responsiveness of the block, I imagine the hooks we have to fire to re-fire the JS will take more time to get right than we think it does.

I truly don't mean to be dismissive of any review points by suggesting features be V2 or V3, but rather just that we have to remember that this is an entirely new block, and although we assume people will love it, but we might be looking at an extremely niche block used once or twice in 10.000 posts.

By putting out a fully functional and nice version, as this PR has, we'll keep things moving and can prioritize our resources where they benefit the most.

And I'm not just saying this because I need Marcus' help on our V2 of the Timeline block ;)

[jeherve]

I logged all issues discussed above in new issues, referenced in this master issue for easier tracking and follow-up: #15742 .

That should allow us to merge this first iteration. Doing that now.

[jeherve]

r207236-wpcom