-
Notifications
You must be signed in to change notification settings - Fork 101
fix: carousel doesn't work inside collapsible/content switch #7047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where carousels inside collapsible sections were non-functional because they initialized while hidden (display:none), causing width and controls to be computed as 0.
Changes:
- Added a
scheduleLayoutUpdatefunction that triggers carousel resize handling after collapsible state changes - Integrated layout updates into all collapsible toggle event handlers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…onents like content switcher
javascript/commons/Carousel.js
Outdated
| if ( typeof ResizeObserver === 'function' ) { | ||
| const ro = new ResizeObserver( () => { | ||
| liquipedia.carousel.scheduleRefresh(); | ||
| } ); | ||
| ro.observe( carouselData.content ); | ||
| carouselData.resizeObserver = ro; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the outer if here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it as safety measure for older browser. ResizeObserver is Baseline Widely available, but still wanted to be safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's widely enough available. It's very close to the levels where webp is today, which we been using for a year or two already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's widely enough available.
Meaning we should drop the outer if? Or good to merge as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Rath means that it should be dropped as it is safe enough to assume that the check is a tautology
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, linter disagrees :( So either keep the check or adjust the linter
|
Change the lint to use |
Thats css linting? Added |
Summary
Resolves #6951
Problem:
Carousels can be initialized while hidden (e.g. inside general-collapsible or ContentSwitch/toggle-area), so initial width measurements are 0 and the controls/scroll behavior don’t work when the content is later revealed.
Fix:
Each carousel now attaches a ResizeObserver to its content and schedules a refresh (handleResize) when its size changes, so it self-heals automatically once it becomes visible.
Also made carousel fader color configurable with a fallback so it's more flexible usable inside components like collapsibles:

How did you test this change?
dev tools on https://liquipedia.net/leagueoflegends/Module:Sandbox/Eetwalt
screenrecording-2026-02-04_15-37-12.mp4