Skip to content

Conversation

@louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Oct 7, 2022

Description

Strongly based on XhmikosR ideas in #28210.

  • Adding a util function to get siblings elements.
  • Adding a js attribute to carousel to fix the number of items in one frame
  • Add some CSS to have items side by side
  • Removed unused class on generated placeholders

Motivation & Context

Seems to be an asked feature.

Type of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Fixes #29130
Fixes #27652

Still todo

Add tests
Better doc
May want to add a focus handle on the first child only (and tabindex=-1 on the remaining) for accessibility purpose (avoid dupes on screen readers)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that those 3 functions should be public ? Or maybe checkItemsBehavior only ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure not public ;)

@GeoSot GeoSot self-assigned this Oct 12, 2022
@patrickhlauke
Copy link
Member

personally not a massive fan of grafting this behaviour on top of the current carousel, as it's a bit clunky already. wondering if in the long run we don't want to change this to a more general purpose "content scroller" type widget.

having said that, this seems to be working ok. even controlling the carousel via JavaScript (e.g. using to(...) to jump to a particular slide) seems to work fine.

i would suggest, for the demo in the docs, not to have it also auto-cycle, as i'd say it makes it more confusing. related, I've been working on a reorganisation/rewrite of the page here #37354

@GeoSot GeoSot force-pushed the main-lmp-carousel-multiple-images branch from e6a68d8 to b88acaa Compare October 26, 2022 21:34
@GeoSot
Copy link
Member

GeoSot commented Oct 26, 2022

@twbs/team any other opinions on this? Should we continue on this idea or not?

Just to be honest, given that there are already two nice carousel implementations, I would leave it as simple as it is possible
https://github.com/kenwheeler/slick/
https://owlcarousel2.github.io/OwlCarousel2/

Note: in case we decide to proceed I am full in to help

@patrickhlauke
Copy link
Member

Note that the description of what this does is not quite accurate. rather than "have multiple items in one slide", this "displays multiple slides at the same time" or similar. And, as suggested, I'd really avoid having data-bs-ride="carousel" (in anticipation of #37354 which I hope will make it in soon)

@louismaximepiton
Copy link
Member Author

I totally understand your concerns about this feature, but is there any update on this ?

@GeoSot
Copy link
Member

GeoSot commented Nov 11, 2022

I gave some time to test the implementation and tried some alternatives.
As much as I like the thought of multiple items, I cannot easily agree with the multiplication of items and the breakpoint check, as it is defined randomly.

I would prefer a more clean way to achieve the multiple items (I tried some things locally, but my css skills didn't help a lot)

In case you can think of a more straight way to achieve it, the breakpoints, could be a configuration array, combined of pixels and number of items

@XhmikosR
Copy link
Member

For what is worth, as much as I believe there are better alternatives out there, it's one of the features we are really missing for so long.

Maybe @mdo can help out with the CSS stuff, but landing such a feature would make a lot of people happy, including myself :)

I had to resort on custom hacked solutions to get this working in the past, and it did work, but official support for this properly would be a huge feature.

}

_checkItemsBehavior() {
if (window.innerWidth >= 768) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be configurable?

@leskam
Copy link

leskam commented Nov 13, 2022

Can't wait for this!

Only thing that would be useful is configurable quantity of item depending on devices resolution (like grid columns), for example:

  • sm, md - 1 item
  • lg - 3 items
  • xl - 4 items

@louismaximepiton
Copy link
Member Author

Hi there, tried some things on this last week, here are my concerns about a different implementation:

  • For a carousel of 2 declared items, if I want to display 3, it's not feasible without automated duplication IMO (e.g. [1|2|1] better than [1|2|])
  • I tried something based on an array of id (e.g. [5,6,1]). Using order helped to have a great visual rendering but I feel like the focus order is awful, so I think this solution should be dropped.
  • I tried something based on relocating the items depending on what needs to be displayed but it would break the existing for what I tried. I maybe missed some things but I'd drop this solution too.

On another topic,
do you think something like data-bs-items="1,2,3,4,5,6" could do the trick for the breakpoints (each number represent the number of items that should be displayed on xs,sm,md,lg,xl,xxl) ?

@leskam
Copy link

leskam commented Nov 21, 2022

On another topic,
do you think something like data-bs-items="1,2,3,4,5,6" could do the trick for the breakpoints (each number represent the number of items that should be displayed on xs,sm,md,lg,xl,xxl) ?

I think for better consistency, configuration should use JSON values like explained in Carousel DOCS, example:
data-bs-config="{"items-breakpoints":{"sm":1, "lg": 3}}"

I would also change the name data-bs-items to data-bs-multiple or data-bs-multiple-items (boolean) for better clarity.

  • For a carousel of 2 declared items, if I want to display 3, it's not feasible without automated duplication IMO (e.g. [1|2|1] better than [1|2|])

In this situation it should only shows 2 items because they are in range (3) - the fact that an item is missing is a fault of developer

@GeoSot
Copy link
Member

GeoSot commented Nov 21, 2022

As a personal preference, I would suggest keeping the code as much as agnostic can be, with less complexity, as the maintenance, can be a real pain

On another topic, do you think something like data-bs-items="1,2,3,4,5,6" could do the trick for the breakpoints (each number represent the number of items that should be displayed on xs,sm,md,lg,xl,xxl) ?

Let let the dev decide the breakpoints, ex: bs-items=2 or bs-items=2,768|3,1200 etc (pick a pattern it suits and can be adapted from other components too on v6)

  • For a carousel of 2 declared items, if I want to display 3, it's not feasible without automated duplication IMO (e.g. [1|2|1] better than [1|2|])

In this case, you should reduce the items programmatically, throw an exception or copy them. Again I would pick the simplest, to reduce items.

@leskam
Copy link

leskam commented Jan 31, 2023

Any chance to implement this feature in 5.3 ?

@louismaximepiton
Copy link
Member Author

I don't think so since the 5.3 is mostly for the dark mode of Bootstrap. Maybe in the next version if I have time to work on it !

@louismaximepiton
Copy link
Member Author

Tried a new version of this, there's still some code generation but way lesser than before. Tweaked a lot of things and didn't manage many others for now. Changed the way to manage the carousel items, indicators etc so it might be a bit broken for now. Just wanted to know if something like that would be better ?

The only other option I can think about is using flexbox (as right now) and force the order property for each child but might be hard to handle the keyboard focus so I'm not that fine using this. Do you have any feedback on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Carousel: allow several items in one slide Carousel multi-items step 1 by 1

6 participants