Add gallery crop option in Inspector #1545

jasmussen · 2017-06-28T10:55:09Z

Screenshot:

jasmussen · 2017-06-28T10:58:37Z

@youknowriad do you have any tips for how I can make the crop enabled by default? I would guess I have to tweak this line: https://github.com/WordPress/gutenberg/pull/1545/files#diff-a7fc58c9ade4d7f0d4de7abd9f6275aaR79

Thanks.

mtias · 2017-06-28T10:59:39Z

Lovely.

mtias · 2017-06-28T11:00:16Z

blocks/library/gallery/index.js

@@ -121,16 +124,24 @@ registerBlockType( 'core/gallery', {
 			controls,
 			focus && images.length > 1 && (
 				<InspectorControls key="inspector">
+					<p className="editor-block-inspector__description">Image galleries are a great way to share groups of pictures on your site.</p>


just mind note: we should create a component for this <BlockDescription> to avoid copying and relying on classes.

I could create a PR for that if nobody is working on it yet. Sounds quite simple.

mtias · 2017-06-28T11:00:39Z

blocks/library/gallery/index.js

@@ -121,16 +124,24 @@ registerBlockType( 'core/gallery', {
 			controls,
 			focus && images.length > 1 && (
 				<InspectorControls key="inspector">
+					<p className="editor-block-inspector__description">Image galleries are a great way to share groups of pictures on your site.</p>
+					<hr />


Is this something we want after every description? Perhaps we should do it with a CSS border in that case.

I figured we'd keep it manual for now, per the idea that we might have blocks with no inspector options, or even use the description elsewhere.

By the way, I've used this pattern for the design:

The italics will come in when the Read More block is merged.

Sounds good.

mtias · 2017-06-28T11:01:41Z

blocks/library/gallery/index.js

 				</InspectorControls>
 			),
-			<div key="gallery" className={ `blocks-gallery align${ align } columns-${ columns }` }>
+			<div key="gallery" className={ `blocks-gallery align${ align } columns-${ columns } crop-${ imageCrop }` }>


Let's start standardizing on is-cropped classes, etc.

mtias · 2017-06-28T11:02:18Z

blocks/library/gallery/style.scss

+			object-fit: cover;
+		}
+
+		// IE10+ hack


Does this apply to IE11 and Edge?

I wasn't able to test this in IE11 yet. Couldn't get docker running. Going to try again with a different approach.

But it should apply to 10 and 11. Edge supports object-fit.

We don't support IE10.

This is IE11. Looking again at hacks:

https://cloudup.com/chrUteDkrD3

Should the comment just say IE11+?

mtias · 2017-06-28T11:31:49Z

Looks good to me. We'll probably want to use the classnames module to handle all the classes we have now, but we can do that later.

jasmussen · 2017-06-28T11:52:34Z

As it turns out, the IE hack needs a little more time in the oven.

swissspidy · 2017-06-28T11:54:21Z

blocks/library/gallery/index.js

@@ -121,16 +124,24 @@ registerBlockType( 'core/gallery', {
 			controls,
 			focus && images.length > 1 && (


I think the description should be shown at all times, not just when there's more than 1 image. Of course, this check makes sense for the controls.

Wonder if we even need a description for this block.

Responded in core slack, I think this is a great place to show contextual help. We want to ensure to have best practices in place that suggest no more than 2 lines, but it feels like it's worth using this spot to show this.

jasmussen · 2017-06-28T13:54:57Z

First, the good news. I pushed a fix that makes this work in IE11 and Edge:

Then the bad news. Turns out object-fit wasn't in Edge after all. It's in the next edge, which is admittedly due this fall, but still. Also, the fix I pushed might be slightly controversial. Please have a look: 84b6e6f

You too, @afercia, if you have time, thank you.

Basically what I'm doing is checking whether the image should be cropped, and if cropped, then output the image as a background. By putting the image file itself inside, but with visibility: hidden;, we ensure that the div containing the background image is actually sized appropriately.

It works perfectly, but it's technically outputting the image twice. I don't know how kosher that is, so please be honest in your code review.

The alternative is to not output the image at all, and instead rely on a fixed aspect ratio, like 3:2 or 1:1 for all cropped thumbnails (which is easy) — we can even let you pick the aspect ratio as Matías suggested. But I'm not sure of the sematic implications of outputting background images. So thoughts appreciated.

brentjett · 2017-06-28T14:07:44Z

The only issue I see with cropping, regardless of how you accomplish it technically, is that not all images have their focal point dead center. This is a broader issue than this ticket, but ideally there would be a way on a per-image basis to specify a focal point or at minimum, where the origin of the image should be anchored. This is more likely a feature that should be provided by the media library and consumed by components like this.

mtias · 2017-06-28T15:45:36Z

@jasmussen I think using images as background could have ramifications with plugins filtering the images (things like photon) as well as hi-dpi / mobile tweaks.

afercia · 2017-06-28T16:56:24Z

@jasmussen well in the editor now the gallery images are just a series of empty <figure> elements. Empty because the images are hidden with visibility: hidden and thus there's really no content that can be read out.

One, very hacky, way to fix this could be (and would need to be tested) giving the figure elements an aria-label but I wouldn't recommend it. The best option is always sticking to the standard semantics and expected behavior, not just for accessibility but also for max interoperability. I wouldn't know how other software or devices would react to empty figure elements.

One more concern is about how themes would then implement this in the front-end. Replicating the hack used in the editor would simply make the images not available to all users. I'm not sure if Google crawls elements hidden with visibility: hidden but I can try to ask to some experts 😏

What would be needed to implement the same layout with some more standard CSS (e.g. transform or something)? Would knowing in advance if the image is landscape or portrait help?

jasmussen · 2017-06-28T17:10:02Z

I'm going to check up on the hidpi stuff, and also see whether we can do the fallback for IE & Edge only.

This is a work in progress. Pushing so as to test in IE.

This is pending more improvements happening separately in #1440 and #1523.

jasmussen · 2017-06-29T09:20:22Z

I reverted back to the very minimal crop feature using object-fit. It has the least repercussions for plugins, and has the least complexity.

It means crop doesn't work in IE11 or Edge. This fall it will work in Edge (which thankfully auto updates).

If we'd like to support IE11, which we probably should at some point, we should explore that in a separate PR. Best option here seems to be exploring utilizing the thumbnails WordPress already provides.

jasmussen · 2017-06-29T09:20:48Z

To put it differently, I think this is ready for final review.

jasmussen added the [Status] In Progress Tracking issues with work in progress label Jun 28, 2017

jasmussen self-assigned this Jun 28, 2017

jasmussen requested a review from youknowriad June 28, 2017 10:58

mtias reviewed Jun 28, 2017

View reviewed changes

jasmussen removed the [Status] In Progress Tracking issues with work in progress label Jun 28, 2017

jasmussen added the [Status] In Progress Tracking issues with work in progress label Jun 28, 2017

swissspidy reviewed Jun 28, 2017

View reviewed changes

swissspidy mentioned this pull request Jun 28, 2017

Add block description component #1555

Merged

jasmussen added 7 commits June 29, 2017 11:14

Crop gallery images by default

389fc17

This is a work in progress. Pushing so as to test in IE.

Polish inspector controls generally.

3969523

This is pending more improvements happening separately in #1440 and #1523.

Make the crop actually work.

09b6c41

Enable crop by default. Props @mtias.

3a60e05

Move to is-cropped classname.

20ce5d8

Fix for IE11.

5ccb8ca

Revert back to object-fit.

5ebb52f

jasmussen force-pushed the add/gallery-crop branch from b71d0f2 to 5ebb52f Compare June 29, 2017 09:17

jasmussen removed the [Status] In Progress Tracking issues with work in progress label Jun 29, 2017

mtias merged commit f12224f into master Jun 29, 2017

mtias deleted the add/gallery-crop branch June 29, 2017 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gallery crop option in Inspector #1545

Add gallery crop option in Inspector #1545

jasmussen commented Jun 28, 2017 •

edited

Loading

jasmussen commented Jun 28, 2017

mtias commented Jun 28, 2017

mtias Jun 28, 2017

swissspidy Jun 28, 2017

mtias Jun 28, 2017

mtias Jun 28, 2017

jasmussen Jun 28, 2017

mtias Jun 28, 2017

mtias Jun 28, 2017

mtias Jun 28, 2017

jasmussen Jun 28, 2017

mtias Jun 28, 2017

jasmussen Jun 28, 2017

mtias Jun 28, 2017

mtias commented Jun 28, 2017

jasmussen commented Jun 28, 2017

swissspidy Jun 28, 2017

mtias Jun 28, 2017

jasmussen Jun 28, 2017

jasmussen commented Jun 28, 2017

brentjett commented Jun 28, 2017

mtias commented Jun 28, 2017

afercia commented Jun 28, 2017

jasmussen commented Jun 28, 2017

jasmussen commented Jun 29, 2017

jasmussen commented Jun 29, 2017

		@@ -121,16 +124,24 @@ registerBlockType( 'core/gallery', {
		controls,
		focus && images.length > 1 && (

Add gallery crop option in Inspector #1545

Add gallery crop option in Inspector #1545

Conversation

jasmussen commented Jun 28, 2017 • edited Loading

jasmussen commented Jun 28, 2017

mtias commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented Jun 28, 2017

jasmussen commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Jun 28, 2017

brentjett commented Jun 28, 2017

mtias commented Jun 28, 2017

afercia commented Jun 28, 2017

jasmussen commented Jun 28, 2017

jasmussen commented Jun 29, 2017

jasmussen commented Jun 29, 2017

jasmussen commented Jun 28, 2017 •

edited

Loading