Skip to content
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

[3] Fix layout shift on thumbnails, quantity #675

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JimmyHoenderdaal
Copy link
Member

Removed absolute php image so we don't get any layout shifts anymore
Did some more tweaking for the quantity and reviews

For the image thumbnails we couldn't merge the PHP and Vue code because of product options

@JimmyHoenderdaal JimmyHoenderdaal changed the title Fix layout shift on thumbnails, reviews, quantity [3] Fix layout shift on thumbnails, reviews, quantity Dec 16, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in https://github.com/rapidez/reviews, not sure if the readme is up-to-date?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have made this PR for reviews rapidez/reviews#28

@JimmyHoenderdaal JimmyHoenderdaal changed the title [3] Fix layout shift on thumbnails, reviews, quantity [3] Fix layout shift on thumbnails, quantity Dec 18, 2024
jordythevulder
jordythevulder previously approved these changes Dec 18, 2024
Copy link
Member

@royduin royduin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to have 1 version without the duplication.

@Jade-GG
Copy link
Collaborator

Jade-GG commented Dec 19, 2024

It should be possible to have 1 version without the duplication.

To a certain degree, we could almost certainly write some pretty ugly frontend that doesn't have duplication. It's made a lot more complex because product options can vary the amount of thumbnails shown. You would have to somehow merge a dynamic v-for with the php @foreach, which can't really be done cleanly.

The way to do this would be to always fill the whole row of thumbnails regardless of how many there actually are on the base product, basically replacing the v-for with a set of v-ifs. Would look something like this (untested code):

@for ($imageId = 0; $imageId < max($breakpoints); $imageId++)
    <button
        @if($imageId >= count($product->images)) v-cloak @endif
        v-if="{{ $imageId }} < images.length"
        v-on:click="change({{ $imageId }})"
        @class([
            $baseClasses,
            'outline outline-1 border-primary' => $imageId == 0,
            'xl:hidden' => $imageId > $breakpoints['xl'],
            'lg:max-xl:hidden' => $imageId > $breakpoints['lg'],
            'md:max-lg:hidden' => $imageId > $breakpoints['md'],
            'sm:max-md:hidden' => $imageId > $breakpoints['sm'],
            'max-sm:hidden' => $imageId > $breakpoints['xs'],
        ])
        v-bind:class="{ 'outline outline-1 border-primary': active == {{ $imageId }} }"
    >
        <img
            src="/storage/{{ config('rapidez.store') }}/resizes/80x80/magento/catalog/product/{{ $product->images[$imageId] }}.webp"
            v-bind:src="'/storage/{{ config('rapidez.store') }}/resizes/80x80/magento/catalog/product/' + images[{{ $imageId }}] + '.webp'"
            alt="{{ $product->name }}"
            class="block max-h-full w-auto object-contain"
            width="80"
            height="80"
        />
        @if ($imageId < count($product->images) - 1)
            <span @class([
                'absolute inset-0 hidden items-center justify-center bg-black/20',
                'xl:flex' => $imageId === $breakpoints['xl'],
                'lg:max-xl:flex' => $imageId === $breakpoints['lg'],
                'md:max-lg:flex' => $imageId === $breakpoints['md'],
                'sm:max-md:flex' => $imageId === $breakpoints['sm'],
                'max-sm:flex' => $imageId === $breakpoints['xs'],
            ])>
                <span
                    class="size-9 flex items-center justify-center rounded-full bg-white text-sm font-bold text shadow-lg"
                    v-text="'+' + images.length - {{ $imageId }} - 1"
                >
                    +{{ count($product->images) - $imageId - 1 }}
                </span>
            </span>
        @endif
    </button>
@endfor

A few hacks in this one. Not sure if this is what we really want.

Copy link
Member

@Roene-JustBetter Roene-JustBetter Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer one version instead of 2 versions if that is possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants