Fix: add null check for activeIndicator in Carousel
Description
Adds null check for activeIndicator to match the pattern used for newActiveIndicator on line 282.
Motivation & Context
While testing carousel behavior with modified DOM, I noticed SelectorEngine.findOne() can return null if there's no active indicator, which would cause an error on classList.remove(). This adds the same safety check that's already in place for newActiveIndicator.
Type of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Refactoring (non-breaking change)
- [ ] Breaking change (fix or feature that would change existing functionality)
Checklist
- [x] I have read the contributing guidelines
- [x] 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
- [x] All new and existing tests passed
Related issues
N/A
This smells like AI.. Why would this be null in the first place? Also, most if not all JS changes need an accompanied test case.
Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.
Happy to add a test case if you think this scenario is worth covering, or close this if you'd rather keep it as-is since it might be an edge case.
Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.
Could you please provide a reproducible use case so we can test it manually, and if possible, include the corresponding unit test in this PR?
Sure thing. This happens when the carousel is initialized programmatically and the indicators element exists but has no active indicator yet. In that case, SelectorEngine.findOne(SELECTOR_ACTIVE, this._indicatorsElement) returns null, and then the code tries to call .classList.remove() on null which throws a TypeError.
I ran into this when I was building indicators dynamically and hadn't synced up the active states properly before the first slide transition.
I've added a test case in the latest commit that covers this scenario - creates a carousel with indicators but none marked as active, then calls next(). Verifies it slides correctly and activates the new indicator without erroring.
The null check matches what's already there for newActiveIndicator on line 282, so it keeps the code consistent.