magesuite / theme-creativeshop

Open Software License 3.0
38 stars 24 forks source link

Product page gallery: image height is no longer hardcoded #46

Closed jtomaszewski closed 3 years ago

jtomaszewski commented 3 years ago

This commit ( cc @krzksz ) removed $loaderPadding from being used in the Magento_Catalog/templates/product/view/gallery.phtml . Now, the product page content "jumps" after image is loaded.

Instead, we could keep on using $loaderPadding so that we "hardcode" the img container's height, so that the browser doesn't have to rerender its' surroundings after image is loaded.

Do you mind bringing it back? I could submit a PR.

mborkowski commented 3 years ago

Hi @jtomaszewski, We have recently removed that in favour of bringing width and height attributes for the image. This operation was done for a couple of reasons, and one of them was to improve also layout shifts. This was done 18 days ago. Please check if this will work for you.

jtomaszewski commented 3 years ago

I think it's better. Although I still have a feeling that the thumbnails under the main picture

image

appear only after client-side render, which causes CLS.

So what we've done, is we've added something like that to product-gallery.scss:


// margin-bottom of `.fotorama__stage`
$fotorama-padding-bottom: 0.4rem;
// height of `.fotorama__nav-wrap`
$fotorama-nav-mobile-height: 35px;
$fotorama-nav-desktop-height: 91px;

.cs-product-gallery {
    $root: &;

    &__placeholder {
        margin: 0 auto;
        padding: ($fotorama-padding-bottom / 2) 0;

        &__inner {
            position: relative;
            overflow: hidden;
        }

        &__inner-before {
            content: '';
            width: 1px;
            margin-left: -1px;
            float: left;
            height: 0;
        }

        &__inner-image {
            position: absolute;
            top: 0;
            left: 0;
            right: 0;
            bottom: 0;
            display: flex;
            align-items: center;
            justify-content: center;
        }

        &:after {
            content: '';
            display: table;
            clear: both;
        }
    }

    &__nav-placeholder {
        &:last-child {
            height: $fotorama-nav-mobile-height - 1px;
            @include media('>=tablet') {
                height: $fotorama-nav-desktop-height - 1px;
            }
        }
    }

    // Always show the navigation,
    // even if it's empty
    // (to minimize layout shift)
    .fotorama {
        min-height: $fotorama-nav-mobile-height - 1px;
        @include media('>=tablet') {
            min-height: $fotorama-nav-desktop-height - 1px;
        }
    }

    // Always show the navigation,
    // even if it's empty
    // (to minimize layout shift)
    .fotorama__nav-wrap {
        display: block !important;
        &[style='display: none;'] {
            visibility: hidden;
        }
        height: $fotorama-nav-mobile-height - 1px;
        @include media('>=tablet') {
            height: $fotorama-nav-desktop-height - 1px;
        }
    }
}

Thanks to it, the row for fotorama thumbnails appears always with constant height (if product has no pictures, it just shows blank space).

Let me know if u'd be interested in a PR for that.

In the meantime, I'll close this issue.

mborkowski commented 3 years ago

Hello, Glad you managed it. Regarding PR offer we're at the stage of thinking how to get rid of fotorama from magesuite and replace it with some lighter and more extensible solution which would also help us to get rid of CLS. Your piece of code would work assuming there's only 1 row of thumbs. In MageSuite we're trying to cover all possible scenarios. If you have an idea how to cover more rows and keep extensibility and simplicity - feel free to open PR. Also please feel free to help us improve other areas ;) Thanks a lot

jtomaszewski commented 3 years ago

U're right; haven't thought of scenario with 2+ rows.

Thanks for the response ;]

groomershop-mt commented 2 years ago

We have applied this patch (which probably will be marged into Magento) and this reduced us CLS almost to 0 on product pages: https://github.com/magento/magento2/pull/33265