-
Notifications
You must be signed in to change notification settings - Fork 799
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 Image Compare block #15598
Add Image Compare block #15598
Conversation
This is an automated check which relies on |
Wonderful, Marcus! Excited about this block.
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:
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:
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. |
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.
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
Great thoughts, Jeremy, I'll respond to the conceptual and design aspects and let Marcus help with the difficult stuff (thanks Marcus).
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?
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. |
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? |
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 |
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: |
Understood — but doesn't AMP entirely strip out external JavaScript, making it possible to serve the button using |
Note that AMP has an 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. |
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! |
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. |
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:
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. |
@jasmussen 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 |
We have a helper for that, it may be useful: 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
|
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. |
093ae32
to
f647cd5
Compare
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. |
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 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.
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.
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?
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.
Good suggestion, we should add this feature. |
Such good reviews, such good conversation. You're all wonderful.
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.
Agreed — for V2.
Seems fine to explore, but I'd go at least V2, possibly even V3.
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 ;) |
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.
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.
r207236-wpcom |
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:
Future Tracking Issues;
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