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

amp-carousel: breakpoint specific configuration #4626

Closed
eike-hass opened this issue Aug 19, 2016 · 27 comments
Closed

amp-carousel: breakpoint specific configuration #4626

eike-hass opened this issue Aug 19, 2016 · 27 comments

Comments

@eike-hass
Copy link

eike-hass commented Aug 19, 2016

Continuing the discussion from #3517: We would like to implement different behavior for amp-carousels on different screens. Would it be possible to have the type property respect media queries aswell?

@rudygalfi
Copy link
Contributor

cc @ericlindley-g

@dvoytenko
Copy link
Contributor

/to @eike-hass Do you already have pages that exhibit this behavior? To confirm, you'd like a page to go from a scrolling carousel to a slideshow and back depending on @media break points? So far we've been pushing these components further apart (even considering splitting them), so could you please describe your case to better understand it?

As it stands, you can currently provide two components and toggle between them using media attribute. That would require you to mostly duplicate content inside them, which is not great, but at least AMP will ensure only the shown elements will be loaded, thus they will not consume more resources.

@eike-hass
Copy link
Author

@dvoytenko: currently there is no example. We imagined something similar to http://kenwheeler.github.io/slick/ (Responsive Display example). I think in parts this could achieved with the current state of amp-carousel, but things like loop, autoplayand possibly indicators would not be possible on the same instance.

@eike-hass eike-hass reopened this Aug 24, 2016
@dvoytenko
Copy link
Contributor

@eike-hass If I understand you right, what you really want is to be able to provide breakpoints for how many slides to show at a time. E.g. it could be 1 slide on max-width:320px, 2 slides for max-width:600px and 3 slides for bigger sizes.

Is this what you want? If that's the case, I think it's a very reasonable extension to our current slideshows.

@eike-hass
Copy link
Author

@dvoytenko exactly. Would it make sense to also make behavior like loop and autoplay adaptive to breakpoints?

@dvoytenko
Copy link
Contributor

@eike-hass loop and autoplay would certainly take multi-slide at a time into account. Or do you mean that you want to turn it on/off based on the breakpoints?

@shimonenator
Copy link

Per @dvoytenko comment in #4642, here's an example that can benefit from this functionality: http://goo.gl/ZoJT73

@eike-hass
Copy link
Author

@dvoytenko Did you consider making breakpoint specific "slides to scroll" available?

@dvoytenko
Copy link
Contributor

@eike-hass Yes! We will do it.

@ericlindley-g @rudygalfi We need to schedule this work.

@ericlindley-g
Copy link
Contributor

cc/ @cramforce

I imagine #5981 affects how we'd approach this. If we deprecate amp-carousel types, the task in this feature request shifts from just changing type on breakpoint, to changing the component entirely on breakpoint.

At that point, I wonder if it's better just to rely on CSS breakpoints to hide/show two different components (though the downside is the extra markup and effort to implement), rather than implementing the behavior in the AMP library. Thoughts?

@cramforce
Copy link
Member

The media attribute is supported for this purpose, but it can really blow up the DOM if applied to things like carousels. Still don't really think it makes sense to support the special case.

@eike-hass
Copy link
Author

eike-hass commented Nov 4, 2016

@ericlindley-g we are using the media attribute with three carousels at the moment which works quite well. We would like to get rid the bloat though.
Reviewing the initial request actually we would need to able to set how many slides should fit in the carousel per "slide" and change that property on specific breakpoints. Would that make it more feasible?

@dvoytenko
Copy link
Contributor

@cramforce and @ericlindley-g what's proposed here is very specific and, I believe, semantically valid use case. We currently have a slides component (<amp-carousel type=slides>) which shows 1 slide at a time. It's a valid preposition to have slides show 2 or 3 slides at a time, from semantical and responsive points of view.

@cramforce
Copy link
Member

Its definitely a good argument to use an attribute to switch type which is
unfortunately terrible for all other reasons.

On Fri, Nov 4, 2016 at 11:23 AM, Dima Voytenko notifications@github.com
wrote:

@cramforce https://github.com/cramforce and @ericlindley-g
https://github.com/ericlindley-g what's proposed here is very specific
and, I believe, semantically valid use case. We currently have a slides
component () which shows 1 slide at a time.
It's a valid preposition to have slides show 2 or 3 slides at a time, from
semantical and responsive points of view.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4626 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT0i1c9U9p0RRJ77LbIDsBhg7D-8Lks5q63gNgaJpZM4JoO5p
.

@dvoytenko
Copy link
Contributor

The type doesn't switch here. It's still a slides component semantically: it still advances the same way (but group by group, not 1 by 1), the slides are still centered, etc.

@cramforce
Copy link
Member

K. Maybe we should do nothing: But allow for a slide source argument to
avoid the children duplication?

On Fri, Nov 4, 2016 at 11:28 AM, Dima Voytenko notifications@github.com
wrote:

The type doesn't switch here. It's still a slides component semantically:
it still advances the same way (but group by group, not 1 by 1), the slides
are still centered, etc.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4626 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT6t_xcJJPX7QHs-m3Kx4-hA7T6Hwks5q63k-gaJpZM4JoO5p
.

@ericlindley-g
Copy link
Contributor

ericlindley-g commented Nov 4, 2016

Ah, I see, there are essentially two behaviors to support for this request:

  1. Support a mechanism that determines the # of slides visible, grouped in a single carousel view
  2. Change the # of slides in a group specified by (1), based on breakpoint

Alternately: an option to reduce the bloat without directly supporting (1) and (2) is to allow for slide source argument.

+1 to reducing the bloat somehow. Something like slide source argument seems like a good idea; on the other hand I could see a show-this-many-slides-at-a-time parameter being useful as an option for amp-slides. Regardless of which of these (or another idea) is the right direction, it seems like a good item to roll into the other carousel improvement work (e.g. better visual affordance for number of items in the carousel). Slotting into "Next" for now, but open to feedback and +1s by anyone else in the community looking to support this use-case.

As a side note, there are other component attributes that could benefit from breakpoint dependencies, as well, like amp-sidebar. Just brainstorming here, but if we build support for (1), one idea we could explore for (2) is a generalized way to change attributes based on breakpoints (or other signals—e.g. if we wanted to hook into the binding work somehow), with a whitelist of applicable component/attributes.

@ericlindley-g ericlindley-g added this to the Next milestone Nov 4, 2016
@camelburrito
Copy link
Contributor

camelburrito commented Feb 11, 2017

We will need to come up with some kind of format to spec the breakpoint config

media: {
      'min-width:1024': {
        slidesToShow: 3,
        slidesToScroll: 3
     },
    'max-width:600': {
      slidesToShow: 2,
      slidesToScroll: 2
    }

Navigation
The slides work based on the scrolling to the right scrollLeft - in the current scenario if there are 4 slides then the corresponding scroll lefts are (0, slidewidth, 2slidewidth, 3slidewidth)

There are 2 code-paths in the existing code.

  • snap points assisted scroll
    • User scrolls - css take over and snaps - update state after a timeout
  • javascript assisted animated-scroll
    • User scrolls - JS takes over animation when scrolling stops - animates to destination - updates state exactly after the animation ends

With the media attribute
Layout

  1. Compute slide width with - total amp-carousel witdth / slidesToShow
  2. resize slide containers
  3. if possible through JS reset the scroll snap points at the appropriate interval (so that it will scroll to the appropriate scroll left based on slidesToShow and slidesToScroll). For example if slidesToShow is 4, and slidesToScroll is 2 and scrollLeft is 0 - then next scroll left would be ,
    SLIDE_WIDTH * slidesToScroll (will need to tweak this at edgecases like what happens if there are only 2 slides remaining in the end - do we show 4 slides or 2 slides ? )
    4)Right now we only have 3 slides displayed at a time (we hide 2 of them - one on the left and one the right) and this lets us swipe in both directions. no we will have to find the number of slides we need to have hidden

Onscroll
find next scrollable point (based on a formula - there is already a fn to getScrollLeftForIndex_ and getNextSlideIndex_(currentScrollLeft) tweaking these functions would basically let us do what we want)

We will also have to figureout what we do when scrolling stops , lets say there are four slides and user scrolled a slide and a half - with the current logic we will snap back to the first slide, but with the new approach since we show multiple slides we could ideally stop at slide 2. This might involve more math but doable

Loops
Loops are done using the order property of the flexbox when the carousel is at its last or first slide, now we will have to update order on multiple slides


Overall

  1. The code could become significantly complex/larger than it already is
  2. I anticipate handling snap-points based solution can get significantly hard so we can disable snaps for carousels with media attribute? and always handle it through the JS assisted approach.
  3. We might need to version it up
  4. Requires extensive regression and testing

Given all these yes we could do this with the existing code without DOM duplication or restructuring

@stephengardner
Copy link

@eike-hass If I understand you right, what you really want is to be able to provide breakpoints for how many slides to show at a time. E.g. it could be 1 slide on max-width:320px, 2 slides for max-width:600px and 3 slides for bigger sizes.

Is this what you want? If that's the case, I think it's a very reasonable extension to our current slideshows.

This is exactly what I am also looking for - looking forward to an update!

@stale
Copy link

stale bot commented Jan 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Jan 30, 2021
@stale stale bot closed this as completed Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Components - Projects
amp-carousel v2 to be verified
UI
  
Canonical AMP
UI - Component
amp-carousel
UI - Customer
Awaiting triage
UI - Type
Awaiting triage
Development

No branches or pull requests