Support carousel - #14
Open
topstar-ai wants to merge 3 commits into
Open
Conversation
IceOnFire
requested changes
Jul 17, 2026
IceOnFire
left a comment
IceOnFire
left a comment
Member
There was a problem hiding this comment.
The carousel looks really good! A few issues I see are:
- The left and right arrows are not vertically centered. Maybe their centering includes the dots, but this makes them a bit weird.
- I like how the arrows fit on top of the slides, but most of the times carousel components show them outside of the scrolling area. Maybe this could be a configurable prop
- The arrows could maybe be implemented as round or ghost Button components, reusing what we have instead of reinventing them? Maybe the look of those arrows could also be configured through a few props
- It would be nice to have an infinite scrolling carousel, which starts back to 1 after 5. Not going back to page 1, maybe changing the items into [2, 3, 4, 5, 1] or something
- Now that I see it, both Carousel and BeforeAfter should be included in the Data Display category, not Controls: Controls are just form controls or other widgets whose primary focus is interaction. Carousel and BeforeAfter are interactive, but their main focus is to display data
- It's very cool to have a bare carousel that can be navigated with the keyboard, but I would expect to be able to drag with the mouse too then
- This is actually a bug: the "Many per view" story doesn't allow to go back once you reach the end
IceOnFire
requested changes
Jul 17, 2026
IceOnFire
left a comment
IceOnFire
left a comment
Member
There was a problem hiding this comment.
Great job, but there's still a couple of issues. Apart from the comments below, I see:
- The Vertical version is squeezed horizontally
- The ManyPerView version seems quite broken when going right
| * @param {number} length | ||
| * @returns {number} | ||
| */ | ||
| export function carouselHome(length) { |
Member
There was a problem hiding this comment.
Every function name is rigthfully an imperative verb in this file. Shouldn't this be getHome, or computeHome? I would leave out the carousel part too, since it resides in a carousel.
Contributor
Author
There was a problem hiding this comment.
I will set name as getHome. ok?
| ): TemplateResult | ||
| renderIndicators(props: CarouselProps): TemplateResult | ||
| renderIndicator(props: CarouselProps, index: number): TemplateResult | ||
| indicatorSelect( |
Member
There was a problem hiding this comment.
I would move this one higher up, in alphabetical order with the rest of the event handlers, and leave the newline between handles and render functions as it was before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.