shoelace icon indicating copy to clipboard operation
shoelace copied to clipboard

feat: add new carousel component

Open alenaksu opened this issue 2 years ago β€’ 4 comments

This PR adds a Carousel component which lets the user to display a set of slides, one or more than one at a time. The implementation uses the scroll-snap feature, so the transition between slides happens when scrolling. The user can navigate the slides using the the prev/next buttons and the pagination indicators as well.

Closes #113

Code exmaple

<sl-carousel heading="Example carousel">
  <sl-carousel-item label="A random picture 1">
    <img src="https://picsum.photos/530/300/?random=1" />
  </sl-carousel-item>
    <sl-carousel-item label="A random picture 2">
    <img src="https://picsum.photos/530/300/?random=2" />
  </sl-carousel-item>
  <sl-carousel-item label="A random picture 3">
    <img src="https://picsum.photos/530/300/?random=3" />
  </sl-carousel-item>
</sl-carousel>

Appereance

image

alenaksu avatar Aug 09 '22 20:08 alenaksu

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
shoelace βœ… Ready (Inspect) Visit Preview Jan 8, 2023 at 0:37AM (UTC)

vercel[bot] avatar Aug 09 '22 20:08 vercel[bot]

I updated the PR with the following:

  • implemented accessibility
  • Added autoplay and autoplay-interval attributes with which is possibile to run the carousel automatically. The autoplay is paused when the user is interacting with the carousel.
  • Added loop attribute that when set allows to user to scroll in the same direction indefinitely.
  • Prev/Next buttons and dot indicators are now optional and can be displayed by setting the show-controls and show-pagination attributes respectively.
  • Added sl-slide-change event that is fired when the active slide changes.
  • Added some API to programmatically interact with the carousel.

Any feedback is more than welcome!

alenaksu avatar Aug 14 '22 11:08 alenaksu

This is awesome! I've been wanting to play with a scroll snap carousel for a while now.

Some initial feedback:

  • Do we need to include the header in the carousel or can that be external to the component?
  • Vertical carousels should accept ArrowUp and ArrowDown. Perhaps up/down and left/right should work regardless of orientation.
  • The scroll hint demo is nice. Could we map it to a custom property instead of setting padding-inline on ::part(slides)? Maybe --scroll-hint-margin or something like that.
  • I'd love to see an example where the prev/next/pagination controls are overlaid on the slides. I expect a lot of users will ask for this, so an example will both prove the concept and answer their questions.
  • The thumbnails in the example should be clickable, otherwise they seem broken.
  • show-controls is a bit generic and will probably be confused with show-pagination. Maybe show-next or even show-arrows would be better. Or maybe show-controls and show-pagination can be combined into a single attribute, e.g. controls="arrows|pagination|both". Not sure...what do you think?
  • Can we rename prevSlide() to previousSlide()?
  • How can I set an active slide? Should there be a setSlide() method? If so, should it accept a number or an element reference β€” or maybe both?
  • I tend to avoid using properties as readonly values. People seem to find them confusing. Should activeSlideElement be a getActiveSlide() method instead? (If it's used internally, we can make it private and remove it from the docs.)
  • When there are no prev/next/pagination controls, the carousel can't be controlled by the keyboard. Would it be better if the carousel accepts focus and the controls don't? That way, users can control it regardless of the control settings and sighted users can use prev/next/pagination as desired.
    • This would be similar to the clear button in <sl-input> where it has tabindex="-1" but it's still discoverable by screen readers β€” it's just not a tab stop.
    • A decent :focus-visible style could wrap the carousel instead of the buttons.
    • I don't believe this will adversely affect accessibility, but it might be worth double checking before doing this.
  • Pagination buttons are a bit dark. Can we try --sl-color-neutral-300 for inactive and --sl-color-neutral-600 for active?

Before shipping:

  • Switch to Unsplash CDN images due to reliability and permissive licensing
  • Add alt text to sample images
  • Move switches from the first example below the carousel for consistency

claviska avatar Aug 16 '22 14:08 claviska

This looks great!! A few small items and suggestions (the feedback above is already so complete πŸ˜€):

  • The aspect ratio of the pagination circles is skewed on mobile (I tested on iOS 15).
  • What happens when images with different aspect ratios are in the same carousel? (This is common with user-contributed content.) Is object-fit: cover used by default? Could a prop be used to set object-fit: none for specific images that should not be resized?
  • Consider a prop on sl-carousel to set a default carousel background (unless it's better to do this in CSS).
  • Consider an option to toggle full-screen mode for the current slide or entire carousel (more of a feature request).

richeklein avatar Aug 20 '22 17:08 richeklein

Hi there, It's been a while since the last update, hope everyone is doing well :)

Here is a quick update with the latest changes:

  • Removed header from carousel

  • Added ArrowUp and ArrowDown in keyboard navigation for respectively previous and next: ArrowRight: move to the next (previous in RTL) slide ArrowLeft: move to the previous (next in RTL) slide ArrowUp: move to the previous slide ArrowDown: move to the next slide

  • Added --scroll-hint-margin css property

  • Added an example with a customized carousel layout. The layout is defined using grid with predefined areas: slides, pagination, arrows, so chaging grid-template-areas css property on the base element, will change the layout. But any better suggestion on this is welcome.

  • Fixed thumbnails navigation in demo

  • Renamed show-controls to show-arrows and updated the corresponding parts/css accordingly. I'm wondering if it could make sense to just call them controls and arrows. (e.g. <sl-carousel controls arrows>)

  • Refactored activeSlideElement getter to be a private method

  • Changed pagination colors as suggested, --sl-color-neutral-300 for inactive and --sl-color-neutral-600 for active.

  • Added --slide-width and --slide-height css properties

  • Switched to unsplash for demo images

  • Added alt text to sample images

  • Moved switches in the demo below the carousel

There are some things that need to be tackled:

@claviska

When there are no prev/next/pagination controls, the carousel can't be controlled by the keyboard. Would it be better if the carousel accepts focus and the controls don't?

Regarding this, I would say that it might make sense, but I'm not sure if it would be 100% right from an accessibility standpoint, because carousels are not really standardized like many other components. Don't we risk to create confusion to keyboard users, who wound not expect those keyboard commands to be there? What do you think?


@richeklein

The aspect ratio of the pagination circles is skewed on mobile (I tested on iOS 15).

Unfortunately, I haven't had a chance to test on an iPhone yet, although it seems to work fine on desktop Safari. May I ask for a screenshot of the problem in the meantime? It would be helpful.

What happens when images with different aspect ratios are in the same carousel? (This is common with user-contributed content.) Is object-fit: cover used by default? Could a prop be used to set object-fit: none for specific images that should not be resized?

There's no object-fit set at the moment, so if you put images with different aspect ratio, they will appear as they are, with their full size. I guess it would be on top of the user to decide the format of the slides. Would you prefer a predefined object-fit insted?

Consider a prop on sl-carousel to set a default carousel background (unless it's better to do this in CSS).

I would definitely suggest using CSS for this purpose.

Consider an option to toggle full-screen mode for the current slide or entire carousel (more of a feature request).

I'm not sure about this, because it means that the carousel will have to implement dialog-like logic, which is perhaps a bit out of its scope. Let me know what you think.

Anyway, it could be rendered fullscreen using just a few lines of css

.fullscreen {
    position: fixed;
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
    z-index: var(--sl-z-index-dialog);
    backdrop-filter: blur(10px);
}

image

alenaksu avatar Oct 02 '22 19:10 alenaksu

Any progress on this? I'm very excited to give it a go!

oelna avatar Nov 02 '22 09:11 oelna

@oelna Please bear with me. I'm currently traveling to sort out what comes next for Shoelace, but I'll be circling back to reviews soon. Please see this article for more information about what's going on:

https://www.abeautifulsite.net/posts/the-future-of-shoelace/

claviska avatar Nov 02 '22 17:11 claviska

@alenaksu Sincere apologies for the delay! I'm ready to commit time to this, but it looks like most of the examples are broken now.

https://shoelace-3w5d57itw-shoelace.vercel.app/components/carousel

Do you mind merging next and moving the remote images into a local folder so it's more portable? They would fit pretty well in docs/assets/examples/carousel/*. Also, can we use images from Unsplash due to their permissive license?

Thanks so much β€” I can't wait to get this merged!

claviska avatar Nov 28 '22 20:11 claviska

Hi @claviska, I merged next and moved the images to the folder you suggested. However, the examples folder was specified in .gitignore, so I removed it in order to commit the images, please let me know if it needs to be restored.

alenaksu avatar Nov 29 '22 23:11 alenaksu

Touch + Dragging

Thanks for the update. Amazing work!

Touch works great, too. We used to spend countless hours trying to make these types of animations work with JS, so it's great to see that this "just works" with CSS! πŸ˜…

I have noticed that many carousels* let you drag the mouse to move the slide, too.

  • https://kenwheeler.github.io/slick/
  • https://splidejs.com/
  • https://glidejs.com/docs/
  • https://ciampo.github.io/macro-carousel/demo/pagination.html (custom element implementation)

*Notably, Bootstrap's carousel does not seem to support this.

Anyways, I wanted to see if this was possible to do in tandem with scroll snapping. I made this pen as a proof of concept:

https://codepen.io/claviska/pen/yLExvdq?editors=0010

There are a couple tricks in there to make it happen, but the code isn't too bad and seems to work well in all browsers. It could probably be updated to account for inertia as well. Do you think something like this would be reasonable to add? (Not sure how it would work with HTML content though…)

The reason I'm bringing this up is because the first thing I do to interact with carousels is click + drag, which we don't currently support. Is that a me thing, or do others get tripped up on that too? I'm a torn on the cost/benefit of this one.

Other Thoughts

  • Can we add Home and End support to pagination? (i.e. jump to first/last slide)
  • Can we rename arrows to navigation? The latter just seems more appropriate.
  • Is there an attribute or another way to identify the current carousel item other than aria-hidden? This can be useful for applying styles/transitions as slides change, but I like to avoid making end users use aria attributes for styling purposes.
    • Since the DOM updates after scrolling, would it be useful to add a "entering" and "exiting" flag as slides intersect the visible area? πŸ€”
    • If attributes don't make sense for this, we could use "states" via data attributes like form controls and convert them to custom states when ElementInternals.states lands.
    • Related: should we add sl-slide-enter and sl-slide-exit events? Will they be useful?
  • Is it currently possible to show multiple slides like this? If not, do you think it would be difficult to add?
  • Should we do anything for users with reduced motion? (e.g. change scroll behavior to instant instead of smooth)
  • We should probably wrap the previous/next icons in a slot so users can override them. We can use the existing icons as fallback content, e.g. <slot name="previous"><sl-icon ...></slot>

Finishing Touches

I know this is still technically a draft, but I'm jotting things down as I come across them to save time later. I'm happy to pick up this work, so don't feel obligated to tackle it if you don't want to!

  • [x] The first example should have all the switches enabled by default to make it more familiar at a glance.
  • [x] I'd like to move the "Custom Layout" example down so it's right before the "Gallery" example.
  • [x] Create --slide-aspect-ratio demo
  • [x] Make the --scroll-hint-margin demo adjustable, perhaps with <sl-range> (document the acceptable values for this property)
  • [x] Create add/remove slides demo like this
  • [x] Carousel Item needs to be documented
  • [x] CSS custom properties need to be documented
  • [x] Is there anything else missing from the docs? (Slots, events, methods, etc.)
  • [x] Improve alt text in sample images to better describe them for unsighted users
  • [x] Add descriptions and more info to all examples.
  • [x] Tests for <sl-carousel> and <sl-carousel-item>

claviska avatar Nov 30 '22 19:11 claviska

Thanks for everything you've done here. This component is really shaping up! Now that the holidays are behind us, let me know where you're at and what (if anything) you need help with so we can get this over the finish line!

Thanks again!

claviska avatar Jan 03 '23 15:01 claviska

Hey, so it should be almost done, these are some things that I planned to do this week:

  • Add an example with custom icons
  • Localize strings
  • Refine documentation
  • Make a full round of accessibility tests
  • Complete unit tests

Also, I experimented a bit with the fullscreen api, it seems a pretty quick thing to implement, but I'd prefer to complete the above first before adding new features.

Regarding the localization stuff or even accessibility tests, for sure I'd appreciate some help

alenaksu avatar Jan 04 '23 10:01 alenaksu

Sincere apologies for the delay in getting this in. I'm going to merge it, add localizations, etc. This will make it into the next release as experimental!

claviska avatar Feb 23 '23 19:02 claviska

@claviska wow this was completely unexpected! πŸ˜„ πŸŽ‰

alenaksu avatar Feb 24 '23 12:02 alenaksu