Amphtml: Support dynamic height for amp-carousel with slides

[milkovsky]

Given: there is an amp-carousel with slides. Each slide can contain both text and images.
Current issue: The height of the carousel is not dynamic, some slides have too many white space, some slide content is cut off.

Actual result:

Expected:

Source code:

<amp-carousel type="slides" layout="responsive"
                  controls loop
                  height="500" width="500" >
  <div class="slide">....</div>
  <div class="slide">....</div>
  <div class="slide">....</div>
</amp-carousel>

[milkovsky]

Here is what I tried in CSS, but these modifications make my AMP page invalid:

i-amphtml-sizer {
  display: none !important;
}

.i-amphtml-slides-container,
.i-amphtml-slide-item,
.i-amphtml-slide-item>* {
  height: auto !important;
}

.i-amphtml-slides-container {
  position: static !important;
  overflow: hidden !important;
}

.i-amphtml-slide-item.i-amphtml-slide-item-show {
  position: absolute !important;
  display: none !important;
}

.i-amphtml-slide-item.i-amphtml-slide-item-show[style="order: 3;"] {
  position: relative !important;
  display: block !important;
}

[choumx]

/to @aghassemi @ericlindley-g

[aghassemi]

@milkovsky Thanks for the report.
I am not sure if shifting content below carousel is the best UX to solve this problem. Alternatives such as supporting expandable "description" same way we do for amp-lightbox-gallery should be considered. /cc @spacedino

/to @ericlindley-g @nainar to see if this fits prioritization in https://github.com/ampproject/amphtml/issues/16508

[ericlindley-g]

Agreed that the UX may not be optimal enough to undertake the effort and take on the maintenance cost of implementing this pattern. Keeping the feature request in order to track/discuss the use case and potentially the pattern itself, and will discuss to see if it fits into some of the current carousel optimization work.

[milkovsky]

Thank your for the reply.
I presented only a basic an example with text and image, but every slide can contain text with multiple images, there also might be an Instagram or Facebook embed, YouTube video etc.

not sure if shifting content below carousel is the best UX

Shifting of content is not a bad idea imo, in addition this shifting is already present in some other AMP components, for example amp-accordion.
What do you consider as a better element for this kind of content?

expandable "description" same way we do for amp-lightbox-gallery

@aghassemi I have checked the amp-lightbox-gallery docu and amp-lightbox-gallery examples, I have found only the aria-describedby, but it does not solve the content presentation problem.
Do you mean something like an amp-accordion here?

[nainar]

@milkovsky's pain points exist without captioned images too.

For e.g. consider a carousel with the following:

• A portrait image,
• A Youtube embed
• A twitter embed

Depending on the layout supported there will either be blank space around the twitter embed (due to the long height set on the carousel to support the portrait image) or some other aesthetic displeasure (the image being cut off).

Centering the elements that are smaller is possible? Adds some aesthetic sugar.
Would agree with @aghassemi and @ericlindley-g that having the arrows jump as content dimensions change is probably not the best solution here.

This site has an example of a responsive height carousel for example: http://kenwheeler.github.io/slick/ (See adaptive height section)

[spacedino]

The few components we have that shift content on the page should be the exception, not the rule. From a UX perspective consistency for the user is what we should optimize for (arrows staying in the same place for example). For some of the things in the list we don't know the size of the object and therefore it's virtually impossible to not have the content shift, although we are working on this cc/ @sparhami . AFAI have seen, amp-lightbox-gallery is the closest we have to solving this problem in a way that gives users control over what content they want to read without affecting the layout of the page.

@milkovsky can you explain more when you say "[amp-lightbox-gallery] doesn't solve the content presentation problem".

[milkovsky]

Hi @spacedino,

"[amp-lightbox-gallery] doesn't solve the content presentation problem".

I mean here that I did not find a solution/example for presentation of the "_HTML content_"* in the amp-lightbox-gallery docu.
*By "_HTML content_" I mean: text and image, text with multiple images, Instagram/Facebook/Twitter embed, YouTube video etc.

In addition amp-lightbox-gallery requires user to do an additional click on the page to open the gallery. That might make sense for an image gallery. but not for other use-cases. For example one might want to write an article "5 easy tips how to lose weight faster". An inline slideshow presented my "expected" example in the GIF image could be a nice way to display the content.

[alexandesigner]

First of all I would like to praise ur work.

Currently facing the same problem. :cry:

I do not know the complexity for the implementation, but maybe the sample cited by @nainar

This site has an example of a responsive height carousel, for example: http://kenwheeler.github.io/slick (See adaptive height section)

We could have an attribute adpativeHeight in component

<amp-carousel type="slides" layout="responsive" adaptiveHeight width="500">

Maybe the ideal behavior resembles the gif presented by @milkovsky

---

UPDATE: I got the result in my use case with only intrisect layout mode 🎉

[irakligeek]

@alexandesigner Can you share your solutions? I'm facing the same problem where I have different height on every slide and unable to set the height of each slide dynamically.

[milkovsky]

@alexandesigner I am also interested, could you please share the solution?

[alexandesigner]

Hey, in my use case I got the result however it is not exactly the same problem reported here, although I also like the idea of adaptive height ..

Just to report, in my case I needed a caption fixed bottom of the image and it was with a height of 20px, out of the space left by the responsive width in some resolutions, I was able to solve the problem only with a few lines of css, using responsive images and using the layout intrisect solved my problem.

I apologize if I have created an expectation around the problem that still exists .. however I also need this solution

[irakligeek]

@alexandesigner This is good when you have just a few paragraphs of copy but in my case, I have the entire articles/recipes that come with each slide, and I found a way to go about this. This is not an ideal solution but it's the best one I could come up with ( actually I found this solution on the web, but can't find the link ). So, you set a fixed height for each slide container and add overflow-y: scroll, so when there is a lot of text you just get a scrollbar. As I said it's not ideal especially for mobile devices but this is what we get with AMP.

Here is the staging link if anyone interested how I approached this:

https://wneenthusiast.staging.wpengine.com/gallery/pair-steak-cabernet/amp/

[Feas1]

i think dynamic height for amp-carousel is important,the scroll for the data overflow is so ugly.

[aghassemi]

The upcoming amp-inline-galley has caption support which is our solution to this problem.

Deduping with #20595

[milkovsky]

@aghassemi thank you for sharing.
The solution sounds promissing. But I would not close this issue, as it is not 100% solved.
In my initial example there was more than one text field: image copyright, inline text, some images have image caption as well. Some slides do not even have images, just text.

See an example:

Please check also the "Expected" gif animation in the issue description.

A 100% soluton would be a slideshow/caousel with a flexible content, not just an image + caption. Think also "text with multiple images, Instagram/Facebook/Twitter embed, YouTube video etc." (https://github.com/ampproject/amphtml/issues/17116#issuecomment-408783254).

FYI I have posted a possible CSS solution in https://github.com/ampproject/amphtml/issues/17116#issuecomment-408091296.