Skip to content

Align tablet landscape feature with main: move constants to theme and remove unused properties#2

Open
Copilot wants to merge 2 commits intofeat/tablet-landscape-uifrom
copilot/address-review-feedback
Open

Align tablet landscape feature with main: move constants to theme and remove unused properties#2
Copilot wants to merge 2 commits intofeat/tablet-landscape-uifrom
copilot/address-review-feedback

Conversation

Copy link

Copilot AI commented Feb 12, 2026

Review feedback from PR DodoraApp#145 identified hardcoded values and properties (columns, containerMaxWidth) that were removed on main but re-added in the feature branch.

Changes

Theme Constants Added

Added six size constants to theme.ts:

  • tabBarHeight, tabBarPaddingTop - used by tab bar layout
  • heroHeight - replaces HERO_HEIGHT constant from ui.ts
  • tabletLandscapeMaxWidth - max width for tablet landscape content
  • landscapePosterWidthMobile, landscapePosterWidthTablet - landscape poster dimensions

Updated Components

  • _layout.tsx: Uses theme.sizes.tabBarHeight and theme.sizes.tabBarPaddingTop; compact tab paddingBottom changed from 2 to 0
  • ResponsiveLayout.tsx: Uses theme.sizes.tabletLandscapeMaxWidth instead of hardcoded 1000
  • DetailsShell.tsx: Uses theme.sizes.landscapePosterWidth{Mobile|Tablet} instead of hardcoded 140/180
  • HeroSection.tsx: Uses theme.sizes.heroHeight instead of importing HERO_HEIGHT constant

Removed Properties

Removed columns and containerMaxWidth from useResponsiveLayout():

  • These were removed on main as unused
  • The PR branch re-added them
  • Removed from ResponsiveLayoutResult interface and implementation
  • Kept new additions: isLandscape, isTablet, isMobile, height
Original prompt

Address the review feedback from PR DodoraApp#145 by making the following changes on the feat/tablet-landscape-ui branch. The upstream main branch has moved these constants into the theme, so all changes must align with the current main branch state.

Important context: On the current main branch of DodoraApp/DodoStream:

  • useResponsiveLayout() in src/hooks/useBreakpoint.ts does NOT have columns, containerMaxWidth, isTablet, isMobile, isLandscape, or height properties. It only returns: breakpoint, isTV, isWide, width, isPlatformTV, splitLayout.
  • The theme (src/theme/theme.ts) has theme.sizes.tabBarHeight (65 scaled), theme.sizes.tabBarPaddingTop (10 scaled), and theme.sizes.heroHeight (500 scaled).
  • The tab bar layout in _layout.tsx uses theme.sizes.tabBarPaddingTop and theme.sizes.tabBarHeight from the theme.

Here are the specific changes required:

1. src/app/(app)/(tabs)/_layout.tsx

  • Review comment: "Do we even need this 2 here? Isn't the marginBottom: -2 on the icon just equaling this out?" on paddingBottom: compactTabs ? 2 : bottom
    • Change the compact paddingBottom from 2 to 0
  • Review comment: "I moved these constants to the theme on master, please adjust this accordingly" on paddingTop: compactTabs ? 4 : 10
    • The non-compact values should use theme constants: theme.sizes.tabBarPaddingTop instead of hardcoded 10, and theme.sizes.tabBarHeight instead of hardcoded 65

2. src/components/layout/ResponsiveLayout.tsx

  • Review comment: "Don't hardcode, use theme" on Math.min(width * 0.75, 1000)
    • Replace the hardcoded 1000 with a theme value. Add a new theme size constant tabletLandscapeMaxWidth with value withScale(1000, scalingFactor) to src/theme/theme.ts in the sizes section, and reference it as theme.sizes.tabletLandscapeMaxWidth in ResponsiveLayout.tsx. The component will need to import useTheme and access the theme.

3. src/components/media/DetailsShell.tsx

  • Review comment: "don't hardcode, use theme" on width={isMobile ? 140 : 180}
    • Replace hardcoded 140 and 180 with theme values. Add two new theme size constants to src/theme/theme.ts: landscapePosterWidthMobile with value withScale(140, scalingFactor) and landscapePosterWidthTablet with value withScale(180, scalingFactor). Use them as theme.sizes.landscapePosterWidthMobile and theme.sizes.landscapePosterWidthTablet.

4. src/components/media/HeroSection.tsx

  • Review comment: "is in theme on master now" on the HERO_HEIGHT constant usage
    • The PR branch uses const heroHeight = isLandscape ? Math.min(HERO_HEIGHT, height * 0.7) : HERO_HEIGHT; with a separately defined HERO_HEIGHT constant. Replace HERO_HEIGHT with theme.sizes.heroHeight (which already exists in the theme on main). Remove the HERO_HEIGHT constant import/definition if it was added by this branch. The hero section on main already uses theme.sizes.heroHeight (see line 165: <Box height={theme.sizes.heroHeight}>).

5. src/hooks/useBreakpoint.ts

  • Review comment: "this was unused and got removed on master" on columns and containerMaxWidth
    • The PR branch added columns and containerMaxWidth useMemo blocks back into useResponsiveLayout(), but these were removed on main. Remove these two useMemo blocks and their return values.
    • The PR branch also added isLandscape, isTablet, isMobile, height to the return type and body. These are new additions from this PR and should stay (they're needed for the landscape feature). But columns and containerMaxWidth must be removed.
    • Make sure the ResponsiveLayoutResult interface is updated accordingly — it should include isLandscape, isTablet, isMobile, height (new from this PR) but NOT columns or containerMaxWidth.

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

… properties

Co-authored-by: Chanceium <71226606+Chanceium@users.noreply.github.com>
Copilot AI changed the title [WIP] Address review feedback for tablet landscape UI changes Align tablet landscape feature with main: move constants to theme and remove unused properties Feb 12, 2026
Copilot AI requested a review from Chanceium February 12, 2026 23:37
@Chanceium Chanceium marked this pull request as ready for review February 12, 2026 23:44
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.

2 participants