WP-144 tp-carousel component #115
Conversation
Signed-off-by: roshniahuja <[email protected]>
Signed-off-by: roshniahuja <[email protected]>
Signed-off-by: roshniahuja <[email protected]>
src/carousel/index.html
Outdated
| <body> | ||
| <main> | ||
| <!--Infinite basic carousel--> | ||
| <tp-carousel infinite="yes" infinite="yes"> |
There was a problem hiding this comment.
@harshdeep-gill Let's remove duplicate attributes.
src/carousel/style.scss
Outdated
|
|
||
| tp-carousel-slides { | ||
| overflow-y: visible; | ||
| overflow-x: auto; |
There was a problem hiding this comment.
@harshdeep-gill Let's align this.
src/carousel/tp-carousel.ts
Outdated
| constructor() { | ||
| // Initialize parent. | ||
| super(); | ||
| this.slides = this.querySelectorAll( 'tp-carousel-slide' ); |
There was a problem hiding this comment.
@harshdeep-gill Let's add an empty line with a comment here.
src/carousel/tp-carousel.ts
Outdated
| super(); | ||
| this.slides = this.querySelectorAll( 'tp-carousel-slide' ); | ||
| this.slideTrack = this.querySelector( 'tp-carousel-slides' ); | ||
| this._observer = new IntersectionObserver( this.attributeChangeOnScroll?.bind( this ), { root: this.slideTrack, threshold: 0.999 } ); |
There was a problem hiding this comment.
@harshdeep-gill Do we need to be that strict to use 0.999 for 100% visibility of the element, or 0.99 should be enough?
Being too strict might not trigger if the root container clips a pixel or two due to rounding or scroll position.
src/carousel/tp-carousel.ts
Outdated
| const index = Array.from( this.slides ).indexOf( entry.target ); | ||
|
|
||
| // Update current slide index based on if it is is right or left scroll. | ||
| if ( index + 1 - ( this.perView - 1 ) > this.currentSlideIndex ) { |
There was a problem hiding this comment.
@harshdeep-gill Let's hold this into a variable above to make it more readable.
index + 1 - ( this.perView - 1 )
src/carousel/tp-carousel.ts
Outdated
| setTimeout( () => { | ||
| // Set the flag to false. | ||
| this.isProgramaticScroll = false; | ||
| }, 500 ); |
There was a problem hiding this comment.
@harshdeep-gill I believe 500ms is dependent on user scroll. What if it takes longer than that to scroll? Can we define a config?
Something to consider for future is setting this to false onscrollEnd event. But it's currently not supported in all browsers.
src/slider/index.html
Outdated
| <main> | ||
| <!--Slider that slides horizontally with responsive settings.--> | ||
| <tp-slider swipe-threshold="210" flexible-height="yes" infinite="yes" swipe="yes" responsive='[{"media":"(min-width: 600px)","flexible-height":"yes","infinite":"no","swipe":"yes","auto-slide-interval":2000,"per-view":1,"step":2},{"media":"(min-width: 300px)","flexible-height":"yes","infinite":"no","swipe":"yes","behaviour":"fade","auto-slide-interval":2000,"per-view":1,"step":1}]'> | ||
| <tp-slider swipe-threshold="210" flexible-height="yes" infinite="yes" swipe="yes" responsive='[{"media":"(min-width: 600px)","flexible-height":"yes","infinite":"no","swipe":"yes","auto-slide-interval":2000,"per-view":2,"step":2},{"media":"(min-width: 300px)","flexible-height":"yes","infinite":"no","swipe":"yes","behaviour":"fade","auto-slide-interval":2000,"per-view":1,"step":1}]'> |
There was a problem hiding this comment.
@harshdeep-gill Please make a separate PR for changes in each component like slider and tooltip resp, so that one pr is responsible for one component and it's easy to track the changes in the future.
| entry: { | ||
| modal: './src/modal/index.ts', | ||
| slider: './src/slider/index.ts', | ||
| newSlider: './src/carousel/index.ts', |
There was a problem hiding this comment.
@harshdeep-gill Can we this be called carousel?
src/carousel/style.scss
Outdated
| overflow-x: auto; | ||
| scroll-snap-type: x mandatory; | ||
| display: flex; | ||
| scrollbar-width: none; |
There was a problem hiding this comment.
@harshdeep-gill Is this tested on on major browser?
src/carousel/tp-carousel.ts
Outdated
| if ( entry.isIntersecting && entry.target instanceof TPCarouselSlideElement && this.slides ) { | ||
| const index = Array.from( this.slides ).indexOf( entry.target ); | ||
|
|
||
| // Update current slide index based on if it is is right or left scroll. |
There was a problem hiding this comment.
@harshdeep-gill Let's remove the duplicate words is is
No description provided.