[Carousel] Wrapper around items

rogemus commented 5 years ago

Bug Report

Current Behavior Right now there is no wrapper around cmp-carousel__item and because of that is impossible to use a third-party library to create a carousel.

Expected behavior/code div around cmp-carousel__item.

Environment

Core Components version 2.3.3-SNAPSHOT

Possible Solution

<div class="cmp-carousel__items">
    <div data-sly-repeat.item="${carousel.items}"
        data-sly-resource="${item.name @ decorationTagName='div'}"
        role="tabpanel"
        class="cmp-carousel__item${itemList.first ? ' cmp-carousel__item--active' : ''}"
        data-cmp-hook-carousel="item">
    </div>
</div>

richardhand commented 5 years ago

@rogemus - could you share more information on the way you are trying to use the component with a 3rd party library and how the div.cmp-carousel__items wrapper helps.

Adding this would force a new version of the component so we should be confident of the value it would bring. There is also the option of providing your own markup in your proxy component to fit your use case.

rogemus commented 5 years ago

hi, thx you for your quick replay :)

we wanted to use one of these libraries:

because they have great support for touch events and mobile devices also they can be easily modified using simple configs. Sadly all of them require to have a wrapper element around items.

richardhand commented 5 years ago

Thanks @rogemus for the feedback, it would be useful for us to know which configurations/options in particular you would like to see supported as perhaps they can make it back into the Core Component.

The example widgets you listed have a simplified markup API where only the panels and their content are provided and the UI is rendered client side - different to the Core Component approach where everything is provided server side. This means you would have to do more in the current Carousel template than simply wrapping the items, including removing many of the UI elements. I would suggest this be done with a bespoke carousel.html in the proxy component.

rogemus commented 5 years ago

The example widgets you listed have a simplified markup API where only the panels and their content are provided and the UI is rendered client side - different to the Core Component approach where everything is provided server side.

That's not true, all of these plugins work with static HTML delivered by the backend. In the end, I opted for using Siema.js + change in HTML on our side. Mostly because of small size and simple API. But in future, I will be changing this to something more advanced to support configuration per breakpoint eg: (example from https://kenwheeler.github.io/slick/):

responsive: [
    {
      breakpoint: 768,
      settings: {
        arrows: false,
        centerMode: true,
        centerPadding: '40px',
        slidesToShow: 3
      }
    },
    {
      breakpoint: 480,
      settings: {
        arrows: false,
        centerMode: true,
        centerPadding: '40px',
        slidesToShow: 1
      }
    }
  ]

Thanks @rogemus for the feedback, it would be useful for us to know which configurations/options in particular you would like to see supported as perhaps they can make it back into the Core Component.

When it comes to options mostly we needed some mobile/touch support in our carousel.

rogemus commented 5 years ago

Side questions:

Do you have any plan to start using ES6 etc. in your components?
Did you experience any performance issue when it comes to multiple MutationObserver on page (one for Carousel, one for Image, one for Search)?
Do you have any plan to extract logic of communication between authoring and component from main js for a component? I'm asking because this logic right now is rendered both on publish instance and on author instance. I mean this:

new Granite.author.MessageChannel("cqauthor", window).subscribeRequestMessage("cmp.panelcontainer", function(message) {
    if (message.data && message.data.type === "cmp-carousel" && message.data.id === myCarouselHTMLElement.dataset["cmpPanelcontainerId"]) {
        if (message.data.operation === "navigate") {
            // handle navigation
        }
    }
});

richardhand commented 5 years ago

Thanks @rogemus, really useful! The ideal is if we can review and incorporate the necessary mobile / touch support back into the existing widget, and generally improve the frontend components such that they cover the majority of project needs.

In the end, I opted for using Siema.js + change in HTML on our side.

Given your requirements this seems like the best approach for now.

But in future, I will be changing this to something more advanced to support configuration per breakpoint

We don't have any plans at the moment to support this, but I believe something similar could be achieved with the responsive grid and separate components & configurations per breakpoint. Or if only style related, with CSS media queries.

Do you have any plan to start using ES6 etc. in your components?

Yes we do, up to now we've supported IE11 for publish side code as we follow the support listed for latest AEM release. But with the recent release of AEM 6.5 (which drops support for IE11 [0]), we will likely undertake a migration.

Did you experience any performance issue when it comes to multiple MutationObserver on page (one for Carousel, one for Image, one for Search)?

We don't have any performance metrics for this, but we already have an issue logged at #458.

Do you have any plan to extract logic of communication between authoring and component from main js for a component?

Yes, we plan to come with a shared UI widget component concept for accessing objects, adding property setters / getters, making public functions available, eventing etc. This will enable us to abstract this author logic. It also ties into the previous point of having a single central mutation observer which would exist in this commons / core UI component concept.

[0] - https://helpx.adobe.com/experience-manager/6-5/sites/deploying/using/technical-requirements.html#SupportedClientPlatforms

richardhand commented 5 years ago

@rogemus, if you don't mind, I will close this issue, we don't have any plans to add the items wrapper in this version of the carousel.

jakublamprecht commented 5 years ago

@richardhand Adding wrapper around those items might be a good idea from semantics point of view, since carousel is a list of slides, which itself implies it would be a good idea to have a wrapper like "ul". This is also suggested by w3c here: https://www.w3.org/WAI/tutorials/carousels/structure/

Also most of carousel solutions I've found have a wrapper for the items (e.g. https://getbootstrap.com/docs/4.0/components/carousel/, https://inclusive-components.design/a-content-slider/, https://amp.dev/documentation/examples/components/amp-carousel/?referrer=ampbyexample.com, https://lightningdesignsystem.com/components/carousel/)

Also, if there was a wrapper, aria-live attribute could be added to it.

Do you think this issue might be worth reopening? Would you be open to contribution?

richardhand commented 5 years ago

@jakublamprecht - adding this would require a new version (v2) of the Carousel, which we don't plan to create any time soon. So unless there's a real practical need for a wrapper, other than it being common, this is low priority. We can however reopen it and I will add the requires new version label.

Would you be open to contribution?

Yes, we're always open to contributions!

richardhand commented 5 years ago

Hi @jakublamprecht, thanks for the pull request. As we won't be introducing a v2 carousel so soon after the v1 with only this change - I will close the PR for now and link it below so we don't forget about it (and can reopen it) when we create a v2. Hope that's okay for you :)

Pull Request: https://github.com/adobe/aem-core-wcm-components/pull/673

adobe / aem-core-wcm-components

[Carousel] Wrapper around items #573

Bug Report