-
Notifications
You must be signed in to change notification settings - Fork 9
feat: improve landscape UI for tablets and phones #145
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,13 +5,18 @@ import { useBreakpoint } from '@/hooks/useBreakpoint'; | |
| import { useSafeAreaInsets } from 'react-native-safe-area-context'; | ||
| import { ResponsiveLayout } from '@/components/layout/ResponsiveLayout'; | ||
| import { NAV_ITEMS } from '@/constants/navigation'; | ||
| import { useWindowDimensions } from 'react-native'; | ||
|
|
||
| export default function TabsLayout() { | ||
| const { bottom } = useSafeAreaInsets(); | ||
| const breakpoint = useBreakpoint(); | ||
| const { width, height } = useWindowDimensions(); | ||
| const isLandscape = width > height; | ||
|
|
||
| // Hide tabs on tablet/TV since we have sidebar | ||
| // Hide tabs on tablet/TV since we have sidebar, | ||
| // and collapse to a compact style on phones in landscape to save vertical space | ||
| const showTabs = breakpoint === 'mobile'; | ||
| const compactTabs = showTabs && isLandscape; | ||
|
|
||
| return ( | ||
| <ResponsiveLayout> | ||
|
|
@@ -23,9 +28,9 @@ export default function TabsLayout() { | |
| backgroundColor: theme.colors.cardBackground, | ||
| borderTopColor: theme.colors.cardBorder, | ||
| borderTopWidth: 1, | ||
| paddingBottom: bottom, | ||
| paddingTop: 10, | ||
| height: 65 + bottom, | ||
| paddingBottom: compactTabs ? 2 : bottom, | ||
| paddingTop: compactTabs ? 4 : 10, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved these constants to the theme on master, please adjust this accordingly |
||
| height: compactTabs ? 46 : 65 + bottom, | ||
| } | ||
| : { | ||
| display: 'none', // Hide tabs on tablet/TV | ||
|
|
@@ -34,8 +39,9 @@ export default function TabsLayout() { | |
| tabBarInactiveTintColor: theme.colors.textSecondary, | ||
| tabBarLabelStyle: { | ||
| fontFamily: theme.fonts.poppinsSemiBold, | ||
| fontSize: 12, | ||
| fontSize: compactTabs ? 10 : 12, | ||
| }, | ||
| tabBarIconStyle: compactTabs ? { marginBottom: -2 } : undefined, | ||
| }}> | ||
| {NAV_ITEMS.map((item) => ( | ||
| <Tabs.Screen | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,20 +12,23 @@ interface ResponsiveLayoutProps { | |
|
|
||
| export const ResponsiveLayout: FC<ResponsiveLayoutProps> = ({ children, maxWidth }) => { | ||
| const breakpoint = useBreakpoint(); | ||
| const { width } = useWindowDimensions(); | ||
| const { width, height } = useWindowDimensions(); | ||
| const isLandscape = width > height; | ||
|
|
||
| // Show sidebar on tablet and TV | ||
| const showSidebar = breakpoint === 'tablet' || breakpoint === 'tv'; | ||
|
|
||
| // Calculate max width for content (50% on large screens) | ||
| // Calculate max width for content | ||
| const contentMaxWidth: number | undefined = | ||
| maxWidth !== undefined | ||
| ? typeof maxWidth === 'number' | ||
| ? maxWidth | ||
| : undefined | ||
| : breakpoint === 'tv' | ||
| ? width * 0.5 | ||
| : undefined; | ||
| : breakpoint === 'tablet' && isLandscape | ||
| ? Math.min(width * 0.75, 1000) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't hardcode, use theme |
||
| : undefined; | ||
|
|
||
| // Handle TV back button to focus sidebar | ||
| const handleBackPress = useCallback(() => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ import { MediaInfo } from '@/components/media/MediaInfo'; | |
| import { useResponsiveLayout } from '@/hooks/useBreakpoint'; | ||
| import { getDetailsCoverSource, getDetailsLogoSource } from '@/utils/media-artwork'; | ||
| import FadeIn from '@/components/basic/FadeIn'; | ||
| import { NO_POSTER_PORTRAIT } from '@/constants/images'; | ||
| import { getImageSource } from '@/utils/image'; | ||
|
|
||
| interface DetailsShellProps { | ||
| media: MetaDetail; | ||
|
|
@@ -25,16 +27,83 @@ interface DetailsShellProps { | |
| export const DetailsShell = memo( | ||
| ({ media, forceTVLayout, headerChildren, children }: PropsWithChildren<DetailsShellProps>) => { | ||
| const theme = useTheme<Theme>(); | ||
| const { isPlatformTV, width } = useResponsiveLayout(); | ||
| const { isPlatformTV, isLandscape, isTablet, isMobile, width } = useResponsiveLayout(); | ||
|
|
||
| const useTVLayout = forceTVLayout ?? isPlatformTV; | ||
| const useLandscapeLayout = !useTVLayout && isLandscape && (isTablet || isMobile); | ||
|
|
||
| const coverSource = useMemo( | ||
| () => getDetailsCoverSource(media.background, media.poster), | ||
| [media.background, media.poster] | ||
| ); | ||
| const logoSource = useMemo(() => getDetailsLogoSource(media.logo), [media.logo]); | ||
|
|
||
| const posterSource = useMemo( | ||
| () => getImageSource(media.poster, NO_POSTER_PORTRAIT), | ||
| [media.poster] | ||
| ); | ||
|
|
||
| // Landscape layout for phones/tablets: poster on the left, info + children on the right | ||
| if (useLandscapeLayout) { | ||
| return ( | ||
| <Box flex={1}> | ||
| <AnimatedImage | ||
| source={coverSource} | ||
| contentFit="cover" | ||
| style={StyleSheet.absoluteFillObject} | ||
| /> | ||
| <LinearGradient | ||
| colors={[theme.colors.semiTransparentBackground, theme.colors.mainBackground]} | ||
| locations={[0, 0.6]} | ||
| style={StyleSheet.absoluteFillObject} | ||
| /> | ||
|
|
||
| <ScrollView> | ||
| <Box flexDirection="row" padding="l" gap="l" position="relative"> | ||
| {/* Left column: poster */} | ||
| <Box width={isMobile ? 140 : 180}> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't hardcode, use theme |
||
| <Box | ||
| borderRadius="l" | ||
| overflow="hidden" | ||
| backgroundColor="cardBackground" | ||
| style={{ aspectRatio: 2 / 3 }}> | ||
| <AnimatedImage | ||
| source={posterSource} | ||
| style={{ width: '100%', height: '100%' }} | ||
| contentFit="cover" | ||
| /> | ||
| </Box> | ||
| </Box> | ||
|
|
||
| {/* Right column: title, info, header children */} | ||
| <Box flex={1} gap="m" justifyContent="center"> | ||
| {!!logoSource ? ( | ||
| <AnimatedImage | ||
| source={logoSource} | ||
| contentFit="contain" | ||
| style={{ | ||
| width: Math.min(width * 0.4, theme.sizes.logoMaxWidth), | ||
| height: theme.sizes.stickyLogoHeight, | ||
| }} | ||
| /> | ||
| ) : ( | ||
| <FadeIn> | ||
| <Text variant="header">{media.name}</Text> | ||
| </FadeIn> | ||
| )} | ||
| <MediaInfo media={media} variant="compact" /> | ||
| {headerChildren} | ||
| </Box> | ||
| </Box> | ||
|
|
||
| <Box paddingHorizontal="l" paddingBottom="l" gap="m"> | ||
| {children} | ||
| </Box> | ||
| </ScrollView> | ||
| </Box> | ||
| ); | ||
| } | ||
|
|
||
| if (!useTVLayout) { | ||
| return ( | ||
| <ScrollView> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { memo, useState, useEffect, useCallback, useRef, useMemo } from 'react'; | ||
| import { StyleSheet } from 'react-native'; | ||
| import { StyleSheet, useWindowDimensions } from 'react-native'; | ||
| import { MotiView } from 'moti'; | ||
| import { LinearGradient } from 'expo-linear-gradient'; | ||
| import { Image } from 'expo-image'; | ||
|
|
@@ -30,6 +30,11 @@ interface HeroSectionProps { | |
| export const HeroSection = memo(({ hasTVPreferredFocus = false }: HeroSectionProps) => { | ||
| const theme = useTheme<Theme>(); | ||
| const { pushToStreams, navigateToDetails } = useMediaNavigation(); | ||
| const { width, height } = useWindowDimensions(); | ||
| const isLandscape = width > height; | ||
|
|
||
| // In landscape, scale hero height to avoid consuming the entire viewport | ||
| const heroHeight = isLandscape ? Math.min(HERO_HEIGHT, height * 0.7) : HERO_HEIGHT; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is in theme on master now |
||
|
|
||
| const [activeIndex, setActiveIndex] = useState(0); | ||
| const autoScrollRef = useRef<ReturnType<typeof setInterval> | null>(null); | ||
|
|
@@ -162,7 +167,7 @@ export const HeroSection = memo(({ hasTVPreferredFocus = false }: HeroSectionPro | |
| } | ||
|
|
||
| return ( | ||
| <Box height={HERO_HEIGHT} width="100%" overflow="hidden"> | ||
| <Box height={heroHeight} width="100%" overflow="hidden"> | ||
| {/* Background Image with Fade Animation */} | ||
| <MotiView | ||
| key={activeItem.id} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,16 +5,33 @@ import theme from '@/theme/theme'; | |
| export type Breakpoint = 'mobile' | 'tablet' | 'tv'; | ||
|
|
||
| /** | ||
| * Hook to detect current breakpoint based on window width | ||
| * Hook to detect current breakpoint based on window width. | ||
| * | ||
| * On TV platforms (`Platform.isTV`), always returns `'tv'` regardless of | ||
| * screen resolution to handle 720p Android TV devices correctly. | ||
| * | ||
| * On non-TV platforms, uses the *shorter* dimension (min of width/height) | ||
| * so that a phone rotated to landscape doesn't suddenly jump to the | ||
| * "tablet" breakpoint, and a tablet in landscape doesn't jump to "tv". | ||
| * This keeps the breakpoint stable across orientation changes while still | ||
| * allowing orientation-specific layout tweaks via `isLandscape` from | ||
| * `useResponsiveLayout`. | ||
| * | ||
| * @returns Current breakpoint: 'mobile' | 'tablet' | 'tv' | ||
| */ | ||
| export function useBreakpoint(): Breakpoint { | ||
| const { width } = useWindowDimensions(); | ||
| const { width, height } = useWindowDimensions(); | ||
|
|
||
| if (width >= theme.breakpoints.tv) { | ||
| // TV platform is always 'tv' regardless of resolution (e.g. 720p = 1280×720) | ||
| if (Platform.isTV) return 'tv'; | ||
|
|
||
| // Use the shorter side so rotation doesn't change breakpoint tier | ||
| const shortSide = Math.min(width, height); | ||
|
|
||
| if (shortSide >= theme.breakpoints.tv) { | ||
| return 'tv'; | ||
| } | ||
| if (width >= theme.breakpoints.tablet) { | ||
| if (shortSide >= theme.breakpoints.tablet) { | ||
| return 'tablet'; | ||
| } | ||
| return 'mobile'; | ||
|
|
@@ -58,11 +75,14 @@ export interface ResponsiveLayoutResult { | |
| /** True for tablet or TV (wide layouts that can show split views) */ | ||
| isWide: boolean; | ||
|
|
||
| /** True when the viewport is wider than it is tall */ | ||
| isLandscape: boolean; | ||
|
|
||
| /** Current window dimensions */ | ||
| width: number; | ||
| height: number; | ||
|
|
||
| /** Number of grid columns appropriate for current breakpoint */ | ||
| /** Number of grid columns appropriate for current breakpoint and orientation */ | ||
| columns: number; | ||
|
|
||
| /** Maximum content width for current breakpoint */ | ||
|
|
@@ -115,20 +135,21 @@ export function useResponsiveLayout(): ResponsiveLayoutResult { | |
| const isTV = breakpoint === 'tv'; | ||
| const isWide = isTablet || isTV; | ||
| const isPlatformTV = Platform.isTV; | ||
| const isLandscape = width > height; | ||
|
|
||
| // Grid columns based on breakpoint | ||
| // Grid columns based on breakpoint + orientation | ||
| const columns = useMemo(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was unused and got removed on master |
||
| if (isTV) return 4; | ||
| if (isTablet) return 3; | ||
| return 2; | ||
| }, [isTV, isTablet]); | ||
| if (isTV) return isLandscape ? 5 : 4; | ||
| if (isTablet) return isLandscape ? 5 : 3; | ||
| return isLandscape ? 3 : 2; | ||
| }, [isTV, isTablet, isLandscape]); | ||
|
|
||
| // Max content width | ||
| const containerMaxWidth = useMemo(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was unused and got removed on master |
||
| if (isTV) return Math.min(width * 0.7, 1200); | ||
| if (isTablet) return Math.min(width * 0.85, 900); | ||
| if (isTablet) return isLandscape ? Math.min(width * 0.9, 1100) : Math.min(width * 0.85, 900); | ||
| return undefined; // Full width on mobile | ||
| }, [isTV, isTablet, width]); | ||
| }, [isTV, isTablet, isLandscape, width]); | ||
|
|
||
| // Content padding | ||
| const contentPadding = useMemo(() => { | ||
|
|
@@ -167,6 +188,7 @@ export function useResponsiveLayout(): ResponsiveLayoutResult { | |
| isTablet, | ||
| isTV, | ||
| isWide, | ||
| isLandscape, | ||
| width, | ||
| height, | ||
| columns, | ||
|
|
||
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 even need this 2 here? Isn't the marginBottom: -2 on the icon just equaling this out?