Skip to content

Fix critical issues: mobile scrolling, auto mode crash, and wrapAround itemsToScroll #527

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devin-ai-integration[bot]
Copy link

@devin-ai-integration devin-ai-integration bot commented Jul 16, 2025

Fix Critical Issues: Mobile Scrolling, Auto Mode Crash, and Enhanced Robustness

Summary

This PR addresses three critical issues in the vue3-carousel component:

  1. Issue Cannot scroll vertically to other section in mobile view. #525: Mobile vertical scrolling blocked - Fixed by changing touch event passive flag to allow native browser scrolling
  2. Issue Carousel breaks page #524: Carousel crashes with itemsToShow: 'auto' - Fixed by adding safety guards and iteration limits to prevent infinite loops
  3. Enhanced robustness - Improved calculateAverage utility with better input validation

The changes are targeted and focused on the specific root causes identified in each issue, with comprehensive test coverage maintained.

Review & Testing Checklist for Human

⚠️ Critical items to verify (medium risk):

  • Test mobile scrolling on actual touch device - Verify that vertical page scrolling works properly while horizontal carousel dragging still functions
  • Test auto mode with various configurations - Set itemsToShow: 'auto' and verify no crashes occur with different slide counts and sizes
  • Verify no regressions in normal carousel behavior - Test standard carousel functionality (navigation, pagination, dragging) to ensure existing features still work
  • Check infinite loop protection doesn't trigger incorrectly - Test edge cases where slides have zero/invalid dimensions to ensure safety guards work appropriately

Recommended test plan:

  1. Run the development playground (pnpm dev) and test mobile scrolling in browser dev tools mobile mode
  2. Test itemsToShow: 'auto' with different slide configurations
  3. Deploy to a test environment and verify mobile scrolling on actual devices
  4. Run full test suite to ensure no regressions

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    useDrag["src/composables/useDrag.ts<br/>Touch event handling"]:::major-edit
    Carousel["src/components/Carousel/Carousel.ts<br/>Main carousel logic"]:::major-edit
    calculateAverage["src/utils/calculateAverage.ts<br/>Number validation utility"]:::minor-edit
    
    touchEvents["Touch Events<br/>(touchstart, touchmove)"]:::context
    autoMode["Auto Mode<br/>(itemsToShow: 'auto')"]:::context
    visibleRange["visibleRange computation<br/>Slide positioning"]:::context
    
    useDrag -.->|"passive: isTouch"| touchEvents
    Carousel -.->|"safety guards"| autoMode
    Carousel -.->|"iteration limits"| visibleRange
    calculateAverage -.->|"isFinite() validation"| Carousel
    
    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit  
        L3[Context/No Edit]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF
Loading

Notes

…robustness

- Fix #525: Set passive: isTouch for touch events to allow native vertical scrolling
- Fix #524: Add safety guards and iteration limits to prevent infinite loops in auto mode
- Enhance calculateAverage with isFinite validation for better robustness

All tests pass, no regressions introduced.

Co-Authored-By: Abdelrahman Ismail <[email protected]>
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0 participants