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

Add responsive breakpoints to product carousel #1458

Conversation

mattcoy-arcticleaf
Copy link
Contributor

What?

The products inside carousels currently get squished together on smaller screens.

This occurs wherever the product carousel component is called. This includes the New Products carousel on the homepage and the Related Products and Customers Also Viewed carousels on the product page.

To replicate, go to the Chambray Towel on the Cornerstone demo theme, decrease the size of your browser window, and notice that the product cards in the Customers Also Viewed carousel do not look so good. See screenshots "Related Products Mobile View - Before" and "Related Products Tablet View - Before" below.

To replicate, go to the Cornerstone demo theme, decrease the size of your browser window, and notice that the product cards under the New Products heading do not look good. See screenshots "New Products Mobile View - Before" and "New Products Tablet View - Before" below.

This PR uses the slick responsive setting in the product carousel component to show fewer products on smaller screen sizes, adding 2 breakpoints to adjust the number of slides to show. These breakpoints use the same values that are specified in the screensizes.scss file. The breakpoints in the slick settings had to be set 1px less than the breakpoints due to how slick works. This change sets the number of products to show on mobile screens (under 551px) to 2, the number to show on table screens (under 801px) to 3, and uses the "columns" value on screens larger than 800px. See all "After" screenshots below. This change has no effect on screens over 800px, so I did not post any desktop screenshots.

Note: This PR eliminates an extra indentation before the card component call.

Tickets / Documentation

Slick carousel: http://kenwheeler.github.io/slick/

Screenshots

New Products Mobile View - Before

new_products_mobile_before

New Products Mobile View - After

new_products_mobile_after

New Products Tablet View - Before

new_products_tablet_before

New Products Tablet View - After

new_products_tablet_after

Related Products Mobile View - Before

related_products_mobile_before

Related Products Mobile View - After

related_products_mobile_after

Related Products Tablet View - Before

related_products_tablet_before

Related Products Tablet View - After

related_products_tablet_after

Use the slick responsive setting in the product carousel component to show fewer products on smaller screen sizes.
@bigbot
Copy link

bigbot commented Feb 21, 2019

Autotagging @bigcommerce/storefront-team @davidchin

@junedkazi
Copy link
Contributor

I believe we had it set up with breakpoints a while ago. But we had to remove it for some other issues we were running into #1371. Also see kenwheeler/slick#2834.

cc @pedelman

@mattcoy-arcticleaf
Copy link
Contributor Author

@junedkazi Was there a particular use-case in which merchants were embedding their storefronts in an iframe that caused this issue?

Would the team be open to an alternative solution here?

I could make “Responsive Carousels” a theme setting, so the merchant would be able to go in and easily enable this.

Or, we could take the slide count out of the data-slick object of the product carousel, and call specific JS within the carousel.js file that checks the width of the window, and loads the number of slides accordingly.

If nothing else, the product carousels should be hidden on mobile, since they are not usable in their current state.

@mattcoy-arcticleaf
Copy link
Contributor Author

@junedkazi any thoughts on the above suggestions? Several merchants have expressed concern over this issue on the forums.

@pedelman
Copy link
Contributor

The issue we ran into was related to displaying slick sliders inside an iframe on mobile safari. This means if the user loads a theme in theme editor in mobile safari, their browser will become unresponsive and end up crashing the browser. This seems to be a known issue in slick, but it was just closed out with no resolution.

To be honest we want to completely remove this library and find a lighter weight solution. Maybe something we look into alternatives and implement the mobile responsive slider using a new library.

Thoughts @junedkazi @mattcoy-arcticleaf?

@mattcoy-arcticleaf
Copy link
Contributor Author

I have some past experience working with other slider plugins (Flexslider, Owl Carousel, etc.), but once I found Slick, I stuck with it. It seemed to be the lightest and had the best code. Of course, there are dozens of slider plugins and I'm sure the perfect one might be out there.

That said, it sounds like this is something that won't be resolved in the near future. Switching to a new library might take a while. Would you be open to any of the alternatives I mentioned as a short-term solution? In the current state, the product carousels are not usable on mobile.

@pedelman

@junedkazi
Copy link
Contributor

@mattcoy-arcticleaf sorry for a delayed response. I was thinking for short term should we switch this block of products to grid layout like we do with Most Popular Products. Also we do have tickets in our backlog to figure out alternative solutions to replace Slick slider.

Thoughts @bookernath @pedelman ?

@bookernath
Copy link
Contributor

@mattcoy-arcticleaf if you want to pull in this fix: kenwheeler/slick#2834 (comment)

... and rebase, I think we can move forward with this. We'll do some testing on our end to make sure the Safari issue is fixed with this patch.

@bookernath
Copy link
Contributor

@mattcoy-arcticleaf I went ahead and pulled this in in #1552

It'll be in our next release.

Thanks for your contribution!

@bookernath bookernath closed this Jul 25, 2019
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