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

fix(Cornnerstone): BCTHEME-52 - Allowing carousel image to stretch distorts image #1711

Merged
merged 8 commits into from
Jul 14, 2020

Conversation

yurytut1993
Copy link
Contributor

What?

homepage_stretch_carousel_images option from config.json didn’t work. We always had streched images and homepage_stretch_carousel_images value is not affected. Now it works - I have updated carousel scss file for to fix it.

Now images are stretching when homepage_stretch_carousel_images: true and contain in case when homepage_stretch_carousel_images: false.

Tested on Chrome, Safari, Firefox, ie11, IOS Safari and Android Chrome.

compat-object-fit class didn’t add in ie11 - I have updated it and now it works. homepage_stretch_carousel_images: false mode looks fine on ie11, homepage_stretch_carousel_images: true stretching images, but images looks distorted (as it is now on prod). Maybe we can find a way to update it in other task. Carousel looks okey even on small width on ie11.

Also I added some javascript to detect vertical or square images and make them looking more nicely and informative on mobile - if image is square or vertical and it has no content block in its slide, we have space to make image wrapper higher.

Tickets / Documentation

Add links to any relevant tickets and documentation.
Ticket

Screenshots (if appropriate)

In Ticket I added videos with prefix final for both homepage_stretch_carousel_images: true and homepage_stretch_carousel_images: false modes. So you can see the implementation. On videos we have the worst case of usage - vertical images, horizontal images, some with text and some without. And all them must be good looking in one carousel

@BC-tymurbiedukhin @golcinho @junedkazi ready for your questions

assets/js/theme/common/carousel.js Show resolved Hide resolved
assets/js/theme/common/carousel.js Outdated Show resolved Hide resolved
assets/js/theme/common/carousel.js Outdated Show resolved Hide resolved
assets/js/theme/common/carousel.js Outdated Show resolved Hide resolved
@bigbot
Copy link

bigbot commented Jul 6, 2020

Autotagging @bigcommerce/storefront-team @davidchin

@yurytut1993
Copy link
Contributor Author

yurytut1993 commented Jul 6, 2020

@golcinho @BC-tymurbiedukhin
To be honest I suppose that my version of implementation is little bit more readable, but you both voted for switch, so let it be switch.
In some cases I am adding variables not for reusage, but to show the purpose of the code.
Also I added some new logic. As you may remember for vertical and square images we make container bigger. The problem is images loading faster then this code works and image is jumping when its height changes. I recorded video (bug_image_jump) and put it to the ticket - better one time to see. New logic fix this problem (fix_image_jump)

@vasiliy-seleznev vasiliy-seleznev merged commit 3cfd1c1 into bigcommerce:master Jul 14, 2020
@junedkazi
Copy link
Contributor

#1677

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.

None yet

6 participants