search

adventistchurch / alps-gutenberg-blocks

A Wordpress plugin to create Gutenberg plugins that work for ALPS.
4 stars 6 forks source link

V3.334 image sizing #16

Closed designerbrent closed 5 years ago

designerbrent commented 5 years ago

These are the changes requested in https://github.com/adventistchurch/alps-wordpress/issues/334

designerbrent commented 5 years ago

I'm having issues with the Gallery block, @davideleuterius. I can select images but they won't upload or get saved/selected to be displayed.

davideleuterius commented 5 years ago

@designerbrent I have pushed a partial fix for the selection / insertion errors. It seems to be working for me when uploading images, or selecting them from the media library for the gallery now.

However there seems to be a weird edge case where if a user has one image set already, and chooses to add by uploading another image, it is generating an error on this. I am still looking into this. The workaround for now is to click the pencil icon at the top of the block, and then click 'Add To Gallery' from there, and it seems to work from there. But this certainly isn't an ideal user flow for this, so I am continuing to look into fixing this part of this issue.

davideleuterius commented 5 years ago

@designerbrent This update should fix any issues with the gallery block. Please review and let me know if this works for you.

designerbrent commented 5 years ago

@davideleuterius I checked out this code in a WP install and gave it a test. When I placed a gallery or 2-up image block, it placed the original, full sized image. When I placed the system image block, it resizes the image that gets placed in there with a smaller thumbnail of the original. (see illustration.)

I thought in an earlier version of this PR you had that working. Was I mistaken?

2019-11-12 at 12 42 PM

davideleuterius commented 5 years ago

@designerbrent For the gallery, it should only be pulling the 'large' image size at this point for any item. I am not sure why it is pulling a full size image other than maybe that image, for some reason, does not have the 'large' file size? I've attached a couple of images to show how the gallery is working on my system.

gallery-1

gallery-2

For the 2-Up block, I should have backported a couple of things I have done in the gallery to it. I will get these in shortly and post this back to you.

designerbrent commented 5 years ago

Ok, clearly I was mistaken. The install I had didn't have any thumbnails, for some reason. I uploaded new images and also told Wordpress to regenerate the thumbnails, and it worked correctly.

I am seeing some odd behavior when I click to add/change the images. When you click on Add to Gallery, it shows no images. If you adjust the dates, they load. I have played with it a bit and it doesn't behave consistently all the time, but usually it shows no images.

2019-11-12 at 4 50 PM

davideleuterius commented 5 years ago

@designerbrent Going to look into the Add to Gallery issue you mentioned in just a moment, but first will give you an update on what I have determined on the image selection behavior and how it has been addressed.

I believe what you are seeing with this (as I am as well) is how retina-enabled browsers pull images from the srcset attribute. I work on a Mac with a retina screen, so the pixel density is higher than someone on an older system, and I imagine you are on a retina screen Mac as well if you have a newer system. The issue is this: we can define what size images should be loaded at what widths, but retina enabled browsers will ignore these settings unless the images have been specifically tailored for retina display.

It seems that if an image does not have a @2x / @3x behind the file name, or a 2x / 3x indicator for the file in the srcset, a browser with a retina screen will do its own calculations, and pull what it thinks is the best appropriate size taking its pixel density into account. So what happens is despite telling the retina browser 'use this file at this screen width' it ignores that, looks at the available files in the srcset, and pulls what would be the 'right' file after it does its calculation. If the browser width is 1440px and you're telling it to use a 700px wide image for this screen width, and you are not providing the 1x / 2x / 3x indicator, it will instead (assuming a 2x device) try to pull a 1400px wide image. If it can't find one that wide, it will just always use the widest it can find. So this isn't even an issue related to ALPS, or the Gutenberg block, but rather that the native WP function to build the srcset doesn't add those indicators.

This is rather frustrating because this means that a LOT of mobile devices with retina screens and other devices / systems with higher pixel densities will be calling on much larger image sizes than what we are actually specifying for that particular width. And this definitely goes against the whole concept of delivering smaller sized files to mobile users.

With that the case, I wrote a small utility function in javascript which will target these 2-up blocks and tweak them with a specific solution for this use case. Because of the two-up configuration, it was best not to modify the native WP srcset generation function because it would have a global impact. Instead this script resets each img src to a smaller image, and also set the sizes attribute to tailored fit for the two-up configuration.

I wish I could do more with this, but unfortunately this is native browser behavior when retina screens are in use.

designerbrent commented 5 years ago

@davideleuterius The 2-up block is looking great. I can add the block, select 2 images, and then edit or change the images.

After reading your note, I tried a few things testing on the Gallery.

davideleuterius commented 5 years ago

@designerbrent That is definitely odd behavior you're experiencing. I looked into this, but:

I am not sure how to address your issues as I can't seem to reproduce them?

designerbrent commented 5 years ago

@davideleuterius After your comments, I decided to try again, but this time with a fresh install. I can't seem to reproduce the problem again, so I'm going to call this a fluke of my old install.

designerbrent commented 5 years ago

@davideleuterius Once the conflict gets resolved with dist/blocks.build.js, I'll this is good for you to merge back to master and call it done.

davideleuterius commented 5 years ago

@designerbrent I have resolved this conflict in this branch. To get the conflict resolved, I used master's blocks.build.js, but that won't contain the fixes for the image sizing issues.Once it is merged with master, we'll need to build it again and commit / push that to catch all of the new code, if that makes sense.

designerbrent commented 5 years ago

Once you get that built, commit it back here and I merge it in.

On Nov 15, 2019, at 14:19, David Eleuterius notifications@github.com wrote:

 @designerbrent I have resolved this conflict in this branch. To get the conflict resolved, I used master's blocks.build.js, but that won't contain the fixes for the image sizing issues.Once it is merged with master, we'll need to build it again and commit / push that to catch all of the new code, if that makes sense.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

davideleuterius commented 5 years ago

@designerbrent Seems it went through ok! Going to jump back on ALPS WP issues. Can you give me your priority on what I should jump on next? Thanks!