Code Review Fixes

src/components/ChannelGallery.tsx

@@ -3,7 +3,7 @@ import styled from '@emotion/styled'
 
 import { ChannelPreviewBase, Gallery } from '@/shared/components'
 import ChannelPreview from './ChannelPreviewWithNavigation'
-import { spacing } from '@/shared/theme'
+import { sizes } from '@/shared/theme'
 import { ChannelFields } from '@/api/queries/__generated__/ChannelFields'
 
 type ChannelGalleryProps = {
@@ -14,13 +14,11 @@ type ChannelGalleryProps = {
 
 const PLACEHOLDERS_COUNT = 12
 
-const trackPadding = `${spacing.xs} 0 0 ${spacing.xs}`
-
 const ChannelGallery: React.FC<ChannelGalleryProps> = ({ title, channels, loading }) => {
   const displayPlaceholders = loading || !channels
 
   return (
-    <Gallery title={title} trackPadding={trackPadding} itemWidth={220} exactWidth={true}>
+    <Gallery title={title} itemWidth={220} exactWidth={true} paddingLeft={sizes.b2} paddingTop={sizes.b2}>
       {displayPlaceholders
         ? Array.from({ length: PLACEHOLDERS_COUNT }).map((_, idx) => (
             <ChannelPreviewBase key={`channel-placeholder-${idx}`} />

src/components/VideoGallery.tsx

@@ -1,8 +1,14 @@
-import React, { useState, useCallback, useMemo } from 'react'
+import React, { useState, useMemo, useCallback } from 'react'
 import { css } from '@emotion/core'
 import styled from '@emotion/styled'
 
-import { breakpointsOfGrid, Gallery, MIN_VIDEO_PREVIEW_WIDTH, VideoPreviewBase } from '@/shared/components'
+import {
+  breakpointsOfGrid,
+  Gallery,
+  MIN_VIDEO_PREVIEW_WIDTH,
+  VideoPreviewBase,
+  CAROUSEL_ARROW_HEIGHT,
+} from '@/shared/components'
 import VideoPreview from './VideoPreviewWithNavigation'
 import { VideoFields } from '@/api/queries/__generated__/VideoFields'
 import { sizes } from '@/shared/theme'
@@ -14,8 +20,6 @@ type VideoGalleryProps = {
 }
 
 const PLACEHOLDERS_COUNT = 12
-const CAROUSEL_ARROW_HEIGHT = 48
-const trackPadding = `${sizes.b2}px 0 0 ${sizes.b2}px`
 
 // This is needed since Gliderjs and the Grid have different resizing policies
 const breakpoints = breakpointsOfGrid({
@@ -32,27 +36,27 @@ const breakpoints = breakpointsOfGrid({
 }))
 
 const VideoGallery: React.FC<VideoGalleryProps> = ({ title, videos, loading }) => {
-  const [imgHeight, setImgHeight] = useState<number>()
-  const imgRef = useCallback((img: HTMLImageElement | null) => {
-    setImgHeight(img?.clientHeight)
+  const [coverHeight, setCoverHeight] = useState<number>()
+  const onCoverResize = useCallback((_, imgHeight) => {
+    setCoverHeight(imgHeight)
   }, [])
-
   const arrowPosition = useMemo(() => {
-    if (!imgHeight) {
+    if (!coverHeight) {
       return
     }
-    const topPx = (imgHeight - CAROUSEL_ARROW_HEIGHT) / 2
+    const topPx = (coverHeight - CAROUSEL_ARROW_HEIGHT) / 2
     return css`
       top: ${topPx}px;
     `
-  }, [imgHeight])
+  }, [coverHeight])
 
   const displayPlaceholders = loading || !videos
 
   return (
     <Gallery
       title={title}
-      trackPadding={trackPadding}
+      paddingLeft={sizes.b2}
+      paddingTop={sizes.b2}
       responsive={breakpoints}
       itemWidth={MIN_VIDEO_PREVIEW_WIDTH}
       arrowCss={arrowPosition}
@@ -61,7 +65,7 @@ const VideoGallery: React.FC<VideoGalleryProps> = ({ title, videos, loading }) =
         ? Array.from({ length: PLACEHOLDERS_COUNT }).map((_, idx) => (
             <StyledVideoPreviewBase key={`video-placeholder-${idx}`} />
           ))
-        : videos!.map((video, idx) => (
+        : videos!.map((video) => (
             <StyledVideoPreview
               id={video.id}
               channelId={video.channel.id}
@@ -73,7 +77,7 @@ const VideoGallery: React.FC<VideoGalleryProps> = ({ title, videos, loading }) =
               duration={video.duration}
               posterURL={video.thumbnailUrl}
               key={video.id}
-              imgRef={idx === 0 ? imgRef : undefined}
+              onCoverResize={onCoverResize}
             />
           ))}
     </Gallery>

src/shared/components/Carousel/Carousel.style.ts

@@ -1,17 +1,29 @@
 import styled from '@emotion/styled'
 import Button from '../Button'
 
+export const CAROUSEL_ARROW_HEIGHT = 48
+
 export const Container = styled.div`
   position: relative;
 `
 
-export const BackgroundGradient = styled.div<{ direction: 'prev' | 'next'; margin: string }>`
+type HasDirection = {
+  direction: 'prev' | 'next'
+}
+
+type HasPadding = {
+  paddingLeft: number
+  paddingTop: number
+}
+
+export const BackgroundGradient = styled.div<HasDirection & HasPadding>`
   position: absolute;
   top: 0;
   left: ${(props) => (props.direction === 'prev' ? 0 : 'auto')};
   right: ${(props) => (props.direction === 'next' ? 0 : 'auto')};
   bottom: 0;
-  margin: ${(props) => props.margin};
+  margin-left: ${(props) => -props.paddingLeft}px;
+  margin-top: ${(props) => -props.paddingTop}px;
   width: 10%;
   z-index: 1;
   background-image: linear-gradient(
@@ -24,8 +36,8 @@ export const BackgroundGradient = styled.div<{ direction: 'prev' | 'next'; margi
 
 export const Arrow = styled(Button)`
   position: absolute;
-  width: 48px;
-  height: 48px;
+  width: ${CAROUSEL_ARROW_HEIGHT}px;
+  height: ${CAROUSEL_ARROW_HEIGHT}px;
   transition: none;
 
   &.disabled {
@@ -46,10 +58,12 @@ export const Arrow = styled(Button)`
   }
 `
 
-export const GliderContainer = styled.div<{ trackPadding: string; margin: string }>`
+export const GliderContainer = styled.div<HasPadding>`
   scrollbar-width: none;
-  padding: ${(props) => props.trackPadding};
-  margin: ${(props) => props.margin};
+  padding-left: ${(props) => props.paddingLeft}px;
+  padding-top: ${(props) => props.paddingTop}px;
+  margin-left: ${(props) => -props.paddingLeft}px;
+  margin-top: ${(props) => -props.paddingTop}px;
 `
 
 export const Track = styled.div`

src/shared/components/Carousel/Carousel.tsx

@@ -6,23 +6,16 @@ import { useGlider, GliderProps } from '../Glider'
 import { Container, GliderContainer, Arrow, Track, BackgroundGradient } from './Carousel.style'
 
 type CarouselProps = {
-  trackPadding?: string
+  paddingLeft?: number
+  paddingTop?: number
   className?: string
   arrowCss?: SerializedStyles
 } & GliderProps
 
-const trackPaddingToMargin = (padding: string) =>
-  padding
-    .split(' ')
-    .map((p) => {
-      const isZero = parseFloat(p) === 0
-      return !isZero ? `-${p}` : p
-    })
-    .join(' ')
-
 const Carousel: React.FC<CarouselProps> = ({
   children,
-  trackPadding = '0',
+  paddingLeft = 0,
+  paddingTop = 0,
   className = '',
   arrowCss,
   slidesToShow = 'auto',
@@ -38,16 +31,15 @@ const Carousel: React.FC<CarouselProps> = ({
     ...gliderOptions,
   })
 
-  const margin = trackPaddingToMargin(trackPadding)
   return (
     <Container {...getContainerProps({ className })}>
       <Arrow {...getPrevArrowProps()} icon="chevron-left" ref={prevArrowRef} css={arrowCss} />
-      <BackgroundGradient direction="prev" margin={margin} />
-      <GliderContainer {...getGliderProps()} trackPadding={trackPadding} margin={margin} ref={ref}>
+      <BackgroundGradient direction="prev" paddingLeft={paddingLeft} paddingTop={paddingTop} />
+      <GliderContainer {...getGliderProps()} paddingLeft={paddingLeft} paddingTop={paddingTop} ref={ref}>
         <Track {...getTrackProps()}>{children}</Track>
       </GliderContainer>
       <Arrow {...getNextArrowProps()} icon="chevron-right" ref={nextArrowRef} css={arrowCss} />
-      <BackgroundGradient direction="next" margin={margin} />
+      <BackgroundGradient direction="next" paddingLeft={paddingLeft} paddingTop={paddingTop} />
     </Container>
   )
 }

src/shared/components/Carousel/index.ts

@@ -1,3 +1,3 @@
 import Carousel from './Carousel'
-
+export { CAROUSEL_ARROW_HEIGHT } from './Carousel.style'
 export default Carousel

src/shared/components/VideoPreview/VideoPreview.tsx

@@ -1,4 +1,4 @@
-import React, { useState, useLayoutEffect } from 'react'
+import React, { useState } from 'react'
 import useResizeObserver from 'use-resize-observer'
 
 import {
@@ -41,7 +41,7 @@ type VideoPreviewProps = {
   showChannel?: boolean
   showMeta?: boolean
   main?: boolean
-  imgRef?: ((instance: HTMLImageElement | null) => void) | React.MutableRefObject<HTMLImageElement | null | undefined>
+  onCoverResize?: (width: number | undefined, height: number | undefined) => void
   onClick?: (e: React.MouseEvent<HTMLElement>) => void
   onChannelClick?: (e: React.MouseEvent<HTMLElement>) => void
   className?: string
@@ -62,29 +62,21 @@ const VideoPreview: React.FC<VideoPreviewProps> = ({
   onClick,
   onChannelClick,
   className,
-  imgRef: externalImgRef,
+  onCoverResize,
 }) => {
   const [scalingFactor, setScalingFactor] = useState(MIN_SCALING_FACTOR)
   const { ref: imgRef } = useResizeObserver<HTMLImageElement>({
     onResize: (size) => {
-      const { width: videoPreviewWidth } = size
+      const { width: videoPreviewWidth, height: videoPreviewHeight } = size
+      if (onCoverResize) {
+        onCoverResize(videoPreviewWidth, videoPreviewHeight)
+      }
       if (videoPreviewWidth && !main) {
         setScalingFactor(calculateScalingFactor(videoPreviewWidth))
       }
     },
   })
 
-  useLayoutEffect(() => {
-    if (externalImgRef) {
-      if (typeof externalImgRef === 'function') {
-        externalImgRef(imgRef.current)
-        return
-      }
-
-      externalImgRef.current = imgRef.current
-    }
-  })
-
   const channelClickable = !!onChannelClick
 
   const handleChannelClick = (e: React.MouseEvent<HTMLElement>) => {

src/shared/components/index.ts

@@ -1,6 +1,6 @@
 export { default as Avatar } from './Avatar'
 export { default as Button } from './Button'
-export { default as Carousel } from './Carousel'
+export { default as Carousel, CAROUSEL_ARROW_HEIGHT } from './Carousel'
 export { default as Dropdown } from './Dropdown'
 export { default as Link } from './Link'
 export { default as NavButton } from './NavButton'