Refactor media components to use AspectRatio wrapper#204
Open
Refactor media components to use AspectRatio wrapper#204
Conversation
- Remove SbMediaWrapper entirely; inline spacing/width logic directly into SbImage, SbVideo, and SbSlideshow using Box - Image: use AspectRatio component when aspectRatio prop is provided, remove wrapperStyle/wrapperClassName props, simplify copyright rendering - Video: use AspectRatio component, consolidate YouTube/Vimeo render paths into a single branch, remove wrapperStyle/wrapperClassName, simplify copyright rendering, strip AI-generated emoji JSDoc - CopyrightLabel: merge inline-white and inline-black into a single block differing only in color, reducing duplication - Tighten aspectRatio prop type on Image to match AspectRatio/Video https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Image accepts "3/4" as an aspect ratio (used by SbImage), so AspectRatio needs to include it in its ratio union type. https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
WalkthroughRemoved the SbMediaWrapper component and inlined its layout behavior into multiple Storyblok UI components; refactored Image/Video/AspectRatio/Grid implementations and typings, updated Storyblok schema sync, and removed related re-exports. (49 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/video/video.tsx (1)
74-76: Remove the redundant ternary expression.The assignment
typeof aspectRatio === "number" ? aspectRatio : aspectRatioalways evaluates toaspectRatioregardless of its type. This appears to be leftover from a refactor.🧹 Proposed simplification
- const ratio = - typeof aspectRatio === "number" ? aspectRatio : aspectRatio; + const ratio = aspectRatio;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/video/video.tsx` around lines 74 - 76, The ternary used to compute ratio is redundant: replace the expression assigning ratio (const ratio = typeof aspectRatio === "number" ? aspectRatio : aspectRatio) with a direct assignment of aspectRatio (e.g., const ratio = aspectRatio) in the component where ratio and aspectRatio are defined so the value is identical but simpler; update any nearby comments if needed and remove the dead conditional logic around ratio calculation in the video component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/video/video.tsx`:
- Around line 74-76: The ternary used to compute ratio is redundant: replace the
expression assigning ratio (const ratio = typeof aspectRatio === "number" ?
aspectRatio : aspectRatio) with a direct assignment of aspectRatio (e.g., const
ratio = aspectRatio) in the component where ratio and aspectRatio are defined so
the value is identical but simpler; update any nearby comments if needed and
remove the dead conditional logic around ratio calculation in the video
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3132f4a0-1223-43ac-ac2f-8e2ce59c402e
📒 Files selected for processing (10)
packages/storyblok-ui/src/components/image/SbImage.tsxpackages/storyblok-ui/src/components/media-wrapper/SbMediaWrapper.tsxpackages/storyblok-ui/src/components/media-wrapper/index.tspackages/storyblok-ui/src/components/slideshow/SbSlideshow.tsxpackages/storyblok-ui/src/components/video/SbVideo.tsxpackages/storyblok-ui/src/index.tspackages/ui/src/components/aspect-ratio/aspect-ratio.tsxpackages/ui/src/components/copyright-label/copyright-label.tsxpackages/ui/src/components/image/image.tsxpackages/ui/src/components/video/video.tsx
💤 Files with no reviewable changes (3)
- packages/storyblok-ui/src/components/media-wrapper/index.ts
- packages/storyblok-ui/src/index.ts
- packages/storyblok-ui/src/components/media-wrapper/SbMediaWrapper.tsx
Slideshow typed aspectRatio as string which caused a type error when passed to Image's strict union type. Align all three sites (sub-props interface, component props, Video cast removal) to the same union. https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/slideshow/slideshow.tsx (1)
72-88:⚠️ Potential issue | 🔴 CriticalType mismatch:
"3/4"is not accepted by theVideocomponent.The
SlideshowVideoSlideProps.aspectRatioincludes"3/4", but theVideocomponent'saspectRatioprop does not accept this value. TheVideocomponent expects"1/1" | "4/3" | "16/9" | "21/9" | "9/16" | number, causing a TS2322 error.Either update the
Videocomponent to accept"3/4"(to align withImageandAspectRatio), or exclude"3/4"fromSlideshowVideoSlideProps:Option A: Exclude "3/4" from SlideshowVideoSlideProps (quick fix)
interface SlideshowVideoSlideProps { videoUrl: string; videoPoster?: string; - aspectRatio: "1/1" | "4/3" | "3/4" | "16/9" | "21/9" | "9/16"; + aspectRatio: "1/1" | "4/3" | "16/9" | "21/9" | "9/16"; }Option B: Update Video component to accept "3/4" (recommended for consistency)
Update the
Videocomponent'saspectRatioprop type inpackages/ui/src/components/video/video.tsxto include"3/4", aligning it with theImageandAspectRatiocomponents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/slideshow/slideshow.tsx` around lines 72 - 88, SlideshowVideoSlideProps includes "3/4" but the Video component's aspectRatio prop type lacks it, causing a TS2322; update the Video component's aspectRatio union type to include "3/4" (in the Video component where the prop type is declared) and ensure any runtime/layout logic in Video (and related helpers) supports the "3/4" ratio so SlideshowVideoSlide (which passes aspectRatio to Video) type-checks correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ui/src/components/slideshow/slideshow.tsx`:
- Around line 72-88: SlideshowVideoSlideProps includes "3/4" but the Video
component's aspectRatio prop type lacks it, causing a TS2322; update the Video
component's aspectRatio union type to include "3/4" (in the Video component
where the prop type is declared) and ensure any runtime/layout logic in Video
(and related helpers) supports the "3/4" ratio so SlideshowVideoSlide (which
passes aspectRatio to Video) type-checks correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36781f5b-270b-4fe4-b437-2293093225f8
📒 Files selected for processing (1)
packages/ui/src/components/slideshow/slideshow.tsx
Slideshow passes "3/4" to Video, which was missing it from its union. Aligns with AspectRatio and Image types. https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
- Widen aspectRatio to string/string|number across AspectRatio, Image, Video, Slideshow — removes repeated type errors from CMS-driven values - Refactor Grid: module-level GRID_COLUMNS_MAP, add as/columnsMd/columnsLg props, responsive columns via CSS custom properties - Export GRID_COLUMNS_MAP and GridColumns from ui package index - Refactor SbGrid to use Grid from @httpjpg/ui instead of reimplementing CSS; unify spacing fields (spacingTop/spacingBottom/paddingTop/paddingBottom) - Refactor SbGridItem: extract pure helper functions, clean buildResponsiveStyles - sync-components: add Storyblok tab panels to grid/grid-item schemas, extract COL_SPAN_OPTIONS constant, align spacing field names https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/image/image.tsx (1)
45-45:⚠️ Potential issue | 🟠 MajorReset
highResLoadedwhensrcchanges.When the same mounted component receives a new
src,highResLoadednever goes back tofalse. Line 101 then keepsshowBlurdisabled for the next asset, so the blur-up path is skipped entirely in slideshow/carousel-style flows.Suggested fix
useEffect(() => { + if (blurOnLoad) { + setHighResLoaded(false); + } + if (!isInView) return; const img = imgRef.current; if (img?.complete && img.naturalHeight !== 0 && img.src === src) { setIsLoaded(true); setHighResLoaded(true); } - }, [isInView, src]); + }, [blurOnLoad, isInView, src]);Also applies to: 92-99, 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image/image.tsx` at line 45, Reset highResLoaded whenever the image source changes: add an effect that watches the prop/state `src` and calls `setHighResLoaded(false)` so the blur-up path can run for a new asset; update the component that declares `const [highResLoaded, setHighResLoaded] = useState(false)` to include a useEffect tied to `src` (or the prop name used) that resets `highResLoaded`, ensuring the logic around `showBlur` (lines referencing `highResLoaded`) behaves correctly on source swaps.
🧹 Nitpick comments (1)
packages/ui/src/components/image/image.tsx (1)
43-45: Remove the unusedisLoadedstate.
isLoadedis never read, so the setters on Lines 58, 63, and 96 only schedule extra renders. Biome is already flagging this, so it’s worth deleting now. As per coding guidelines,**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, and Typescript. Highlight any deviations.Suggested cleanup
- const [isLoaded, setIsLoaded] = useState(false); const [isInView, setIsInView] = useState(false); const [highResLoaded, setHighResLoaded] = useState(false); @@ - setIsLoaded(true); onLoad?.(e); @@ - setIsLoaded(true); setHighResLoaded(true); @@ - setIsLoaded(true); setHighResLoaded(true);Also applies to: 56-64, 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image/image.tsx` around lines 43 - 45, Remove the unused React state variable isLoaded from packages/ui/src/components/image/image.tsx: delete the useState declaration const [isLoaded, setIsLoaded] = useState(false) and remove all calls to setIsLoaded (e.g., in the image loading handlers where setIsLoaded(...) is invoked), leaving only the existing isInView and highResLoaded logic; ensure no references remain to isLoaded or setIsLoaded so the component compiles without unused variables and extra renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/storyblok-ui/src/components/grid-item/SbGridItem.tsx`:
- Around line 52-74: The responsive media-query rules from buildResponsiveStyles
can't override the inline grid styles set on the GridItem, so change the
GridItem rendering logic to avoid inline grid-column/grid-row when responsive
spans are present: update the component where inline styles are set (the code
that uses colSpan, rowSpan at render time) to check for
colSpanMd/colSpanLg/rowSpanMd/rowSpanLg and if any exist, do not emit
grid-column or grid-row inline; instead include a non-media base rule in
buildResponsiveStyles (use the same data-sb-grid-item="${uid}" selector) for the
default colSpan/rowSpan and keep the media-query rules for md/lg, or
alternatively move all span declarations into buildResponsiveStyles and always
rely on its base + media rules; reference buildResponsiveStyles,
colSpan/rowSpan, colSpanMd/colSpanLg, rowSpanMd/rowSpanLg, colSpanToCss,
parseRowSpan and the data-sb-grid-item attribute when making the change.
In `@packages/storyblok-ui/src/components/grid/SbGrid.tsx`:
- Around line 27-30: SbGrid is currently only reading the new spacing props
(spacingTop, spacingBottom, paddingTop, paddingBottom) which breaks render for
existing stories that still use legacy keys (mt, mb, pt, pb); update the prop
handling in the SbGrid component (and any related helpers used at the other
spots referenced) to prefer the new keys but fall back to the legacy keys when
the new ones are undefined (e.g., spacingTop ?? mt, spacingBottom ?? mb,
paddingTop ?? pt, paddingBottom ?? pb), and ensure any style/class computation
uses these resolved values so both old and new stories render correctly until
the content migration runs.
- Around line 4-7: The file imports unused symbols GRID_COLUMNS_MAP and css
which cause noUnusedImports errors; remove GRID_COLUMNS_MAP and css from the
import line so only the needed symbols (Grid, type GridColumns, type GridProps)
and the existing SbBlokData/memo imports remain, ensuring the import statement
in SbGrid.tsx is cleaned up to match actual usage after the refactor.
In `@packages/ui/src/components/image/image.tsx`:
- Around line 101-123: The main image is forced to opacity 0 until load even
when there is no blur placeholder; update the main <img> rendering logic (the
element that uses highResLoaded to set style opacity) to base the initial hidden
state on showBlur instead of just highResLoaded—i.e., only set opacity to 0
while loading when showBlur is true, otherwise render the main image fully
opaque; apply the same change where the main image opacity is handled around the
other occurrence (lines 145–149) so both places use showBlur || equivalent check
(using blurOnLoad/blurDataURL via showBlur).
---
Outside diff comments:
In `@packages/ui/src/components/image/image.tsx`:
- Line 45: Reset highResLoaded whenever the image source changes: add an effect
that watches the prop/state `src` and calls `setHighResLoaded(false)` so the
blur-up path can run for a new asset; update the component that declares `const
[highResLoaded, setHighResLoaded] = useState(false)` to include a useEffect tied
to `src` (or the prop name used) that resets `highResLoaded`, ensuring the logic
around `showBlur` (lines referencing `highResLoaded`) behaves correctly on
source swaps.
---
Nitpick comments:
In `@packages/ui/src/components/image/image.tsx`:
- Around line 43-45: Remove the unused React state variable isLoaded from
packages/ui/src/components/image/image.tsx: delete the useState declaration
const [isLoaded, setIsLoaded] = useState(false) and remove all calls to
setIsLoaded (e.g., in the image loading handlers where setIsLoaded(...) is
invoked), leaving only the existing isInView and highResLoaded logic; ensure no
references remain to isLoaded or setIsLoaded so the component compiles without
unused variables and extra renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daf77644-4aa5-430e-8761-ef72689f8e28
📒 Files selected for processing (9)
packages/storyblok-sync/scripts/sync-components.tspackages/storyblok-ui/src/components/grid-item/SbGridItem.tsxpackages/storyblok-ui/src/components/grid/SbGrid.tsxpackages/ui/src/components/aspect-ratio/aspect-ratio.tsxpackages/ui/src/components/grid/grid.tsxpackages/ui/src/components/grid/index.tspackages/ui/src/components/image/image.tsxpackages/ui/src/components/slideshow/slideshow.tsxpackages/ui/src/components/video/video.tsx
✅ Files skipped from review due to trivial changes (3)
- packages/ui/src/components/slideshow/slideshow.tsx
- packages/ui/src/components/grid/index.ts
- packages/ui/src/components/video/video.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/aspect-ratio/aspect-ratio.tsx
Comment on lines
+52
to
+74
| function buildResponsiveStyles(uid: string, blok: SbGridItemProps["blok"]): string { | ||
| const { colSpanMd, colSpanLg, rowSpanMd, rowSpanLg } = blok; | ||
| const rules: string[] = []; | ||
|
|
||
| if (colSpanMd || rowSpanMd) { | ||
| const declarations: string[] = []; | ||
| if (colSpanMd) declarations.push(`grid-column:${colSpanToCss(colSpanMd)}`); | ||
| if (rowSpanMd) declarations.push(`grid-row:span ${parseRowSpan(rowSpanMd) ?? 1}`); | ||
| rules.push( | ||
| `@media(min-width:768px){[data-sb-grid-item="${uid}"]{${declarations.join(";")}}}` | ||
| ); | ||
| } | ||
|
|
||
| if (colSpanLg || rowSpanLg) { | ||
| const declarations: string[] = []; | ||
| if (colSpanLg) declarations.push(`grid-column:${colSpanToCss(colSpanLg)}`); | ||
| if (rowSpanLg) declarations.push(`grid-row:span ${parseRowSpan(rowSpanLg) ?? 1}`); | ||
| rules.push( | ||
| `@media(min-width:1024px){[data-sb-grid-item="${uid}"]{${declarations.join(";")}}}` | ||
| ); | ||
| } | ||
|
|
||
| return rules.join(""); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
fd "SbGridItem.tsx" --type fRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
packages/storyblok-ui/src/components/grid-item/SbGridItem.tsx
Script executed:
fd "grid-item.tsx" --type fRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
packages/ui/src/components/grid/grid-item.tsx
Script executed:
wc -l packages/storyblok-ui/src/components/grid-item/SbGridItem.tsxRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
121 packages/storyblok-ui/src/components/grid-item/SbGridItem.tsx
Script executed:
cat -n packages/storyblok-ui/src/components/grid-item/SbGridItem.tsxRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
1 "use client";
2
3 import { type BlokItem, DynamicRender } from "@httpjpg/storyblok-utils";
4 import { GridItem } from "@httpjpg/ui";
5 import type { SbBlokData } from "@storyblok/react/rsc";
6 import { memo } from "react";
7 import { useStoryblokEditable } from "../../lib/use-storyblok-editable";
8
9 export interface SbGridItemProps {
10 blok: {
11 _uid: string;
12 content?: SbBlokData[];
13 colSpan?: string;
14 colSpanMd?: string;
15 colSpanLg?: string;
16 rowSpan?: string;
17 rowSpanMd?: string;
18 rowSpanLg?: string;
19 colStart?: string;
20 colEnd?: string;
21 rowStart?: string;
22 rowEnd?: string;
23 };
24 }
25
26 function parseColSpan(
27 value?: string,
28 ): 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | "full" | undefined {
29 if (!value) return undefined;
30 if (value === "full") return "full";
31 const num = Number.parseInt(value, 10);
32 if (Number.isNaN(num) || num < 1 || num > 12) return undefined;
33 return num as 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12;
34 }
35
36 function parseRowSpan(value?: string): number | undefined {
37 if (!value) return undefined;
38 const num = Number.parseInt(value, 10);
39 return !Number.isNaN(num) && num >= 1 ? num : undefined;
40 }
41
42 function parsePosition(value?: string): number | undefined {
43 if (!value) return undefined;
44 const num = Number.parseInt(value, 10);
45 return Number.isNaN(num) ? undefined : num;
46 }
47
48 function colSpanToCss(value: string): string {
49 return value === "full" ? "1 / -1" : `span ${parseColSpan(value) ?? 1}`;
50 }
51
52 function buildResponsiveStyles(uid: string, blok: SbGridItemProps["blok"]): string {
53 const { colSpanMd, colSpanLg, rowSpanMd, rowSpanLg } = blok;
54 const rules: string[] = [];
55
56 if (colSpanMd || rowSpanMd) {
57 const declarations: string[] = [];
58 if (colSpanMd) declarations.push(`grid-column:${colSpanToCss(colSpanMd)}`);
59 if (rowSpanMd) declarations.push(`grid-row:span ${parseRowSpan(rowSpanMd) ?? 1}`);
60 rules.push(
61 `@media(min-width:768px){[data-sb-grid-item="${uid}"]{${declarations.join(";")}}}`
62 );
63 }
64
65 if (colSpanLg || rowSpanLg) {
66 const declarations: string[] = [];
67 if (colSpanLg) declarations.push(`grid-column:${colSpanToCss(colSpanLg)}`);
68 if (rowSpanLg) declarations.push(`grid-row:span ${parseRowSpan(rowSpanLg) ?? 1}`);
69 rules.push(
70 `@media(min-width:1024px){[data-sb-grid-item="${uid}"]{${declarations.join(";")}}}`
71 );
72 }
73
74 return rules.join("");
75 }
76
77 export const SbGridItem = memo(function SbGridItem({ blok }: SbGridItemProps) {
78 const {
79 content,
80 colSpan,
81 colSpanMd,
82 colSpanLg,
83 rowSpan,
84 rowSpanMd,
85 rowSpanLg,
86 colStart,
87 colEnd,
88 rowStart,
89 rowEnd,
90 } = blok;
91
92 const editableProps = useStoryblokEditable(blok);
93 const hasPositioning = colStart || colEnd || rowStart || rowEnd;
94 const hasResponsive = colSpanMd || colSpanLg || rowSpanMd || rowSpanLg;
95 const responsiveCss = hasResponsive ? buildResponsiveStyles(blok._uid, blok) : null;
96
97 return (
98 <>
99 {responsiveCss && (
100 <style
101 // biome-ignore lint/security/noDangerouslySetInnerHtml: controlled values — grid span/position strings
102 dangerouslySetInnerHTML={{ __html: responsiveCss }}
103 />
104 )}
105 <GridItem
106 {...editableProps}
107 colSpan={hasPositioning ? undefined : parseColSpan(colSpan)}
108 rowSpan={hasPositioning ? undefined : parseRowSpan(rowSpan)}
109 colStart={parsePosition(colStart)}
110 colEnd={parsePosition(colEnd)}
111 rowStart={parsePosition(rowStart)}
112 rowEnd={parsePosition(rowEnd)}
113 data-sb-grid-item={blok._uid}
114 >
115 {content && content.length > 0 && (
116 <DynamicRender data={content as BlokItem[]} />
117 )}
118 </GridItem>
119 </>
120 );
121 });
Script executed:
wc -l packages/ui/src/components/grid/grid-item.tsxRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
123 packages/ui/src/components/grid/grid-item.tsx
Script executed:
cat -n packages/ui/src/components/grid/grid-item.tsxRepository: dmnktoe/httpjpg
Repository: dmnktoe/httpjpg
Exit code: 0
stdout:
1 "use client";
2
3 import type { HTMLAttributes, ReactNode } from "react";
4 import { forwardRef } from "react";
5 import { css, cx } from "styled-system/css";
6 import type { SystemStyleObject } from "styled-system/types";
7
8 export interface GridItemProps
9 extends Omit<HTMLAttributes<HTMLDivElement>, "css"> {
10 /**
11 * Grid item content
12 */
13 children?: ReactNode;
14 /**
15 * Column span (1-12 or "full" for full width)
16 * `@default` 1
17 */
18 colSpan?: 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | "full";
19 /**
20 * Row span
21 * `@default` 1
22 */
23 rowSpan?: number;
24 /**
25 * Column start position
26 */
27 colStart?: number;
28 /**
29 * Column end position
30 */
31 colEnd?: number;
32 /**
33 * Row start position
34 */
35 rowStart?: number;
36 /**
37 * Row end position
38 */
39 rowEnd?: number;
40 /**
41 * Additional Panda CSS styles
42 */
43 css?: SystemStyleObject;
44 }
45
46 /**
47 * GridItem component - Individual grid cell with precise control
48 *
49 * Use inside Grid to control specific positioning, spanning, and overlap.
50 * Perfect for creating complex magazine-style layouts with overlapping elements.
51 *
52 * `@example`
53 * ```tsx
54 * <GridItem colSpan={6} rowSpan={2}>
55 * Spans 6 columns and 2 rows
56 * </GridItem>
57 *
58 * // Precise positioning
59 * <GridItem colStart={2} colEnd={5} rowStart={1} rowEnd={3}>
60 * Custom grid area
61 * </GridItem>
62 *
63 * // Full width
64 * <GridItem colSpan="full">
65 * Full width content
66 * </GridItem>
67 * ```
68 */
69 export const GridItem = forwardRef<HTMLDivElement, GridItemProps>(
70 (
71 {
72 children,
73 colSpan = 1,
74 rowSpan = 1,
75 colStart,
76 colEnd,
77 rowStart,
78 rowEnd,
79 className,
80 style,
81 css: cssProp,
82 ...props
83 },
84 ref,
85 ) => {
86 // Calculate grid positioning
87 // For simple spans, Panda will use generated utility classes
88 // For complex positioning (start/end), we use inline styles (unavoidable)
89 const gridColumn =
90 colSpan === "full" && !colStart
91 ? "1 / -1"
92 : colStart
93 ? `${colStart} / ${colEnd || (colSpan === "full" ? "-1" : `span ${colSpan}`)}`
94 : `span ${colSpan}`;
95
96 const gridRow = rowStart
97 ? `${rowStart} / ${rowEnd || `span ${rowSpan}`}`
98 : `span ${rowSpan}`;
99
100 // Combine base styles with custom css prop
101 const styles = css({
102 boxSizing: "border-box",
103 ...cssProp,
104 });
105
106 return (
107 <div
108 ref={ref}
109 className={cx(styles, className)}
110 style={{
111 gridColumn,
112 gridRow,
113 ...style,
114 }}
115 {...props}
116 >
117 {children}
118 </div>
119 );
120 },
121 );
122
123 GridItem.displayName = "GridItem";
Responsive span overrides are ineffective due to CSS cascade precedence.
The buildResponsiveStyles() function generates media query rules with attribute selectors, but GridItem always sets grid-column and grid-row as inline styles (lines 110-114 in grid-item.tsx). Inline styles have higher CSS specificity than stylesheet rules, so the responsive rules from colSpanMd, colSpanLg, rowSpanMd, and rowSpanLg cannot override them. To support responsive span changes, either move the grid span logic into the media query styles entirely, or apply responsive styles differently in GridItem itself.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 58-58: Biome lint (style/useBlockStatements): block statements are preferred in this position for if (colSpanMd) declarations.push(...).
[warning] 59-59: Biome lint (style/useBlockStatements): block statements are preferred in this position for if (rowSpanMd) declarations.push(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/storyblok-ui/src/components/grid-item/SbGridItem.tsx` around lines
52 - 74, The responsive media-query rules from buildResponsiveStyles can't
override the inline grid styles set on the GridItem, so change the GridItem
rendering logic to avoid inline grid-column/grid-row when responsive spans are
present: update the component where inline styles are set (the code that uses
colSpan, rowSpan at render time) to check for
colSpanMd/colSpanLg/rowSpanMd/rowSpanLg and if any exist, do not emit
grid-column or grid-row inline; instead include a non-media base rule in
buildResponsiveStyles (use the same data-sb-grid-item="${uid}" selector) for the
default colSpan/rowSpan and keep the media-query rules for md/lg, or
alternatively move all span declarations into buildResponsiveStyles and always
rely on its base + media rules; reference buildResponsiveStyles,
colSpan/rowSpan, colSpanMd/colSpanLg, rowSpanMd/rowSpanLg, colSpanToCss,
parseRowSpan and the data-sb-grid-item attribute when making the change.
Comment on lines
+4
to
7
| import { type GridColumns, type GridProps, Grid, GRID_COLUMNS_MAP } from "@httpjpg/ui"; | ||
| import type { SbBlokData } from "@storyblok/react/rsc"; | ||
| import { memo } from "react"; | ||
| import { css } from "styled-system/css"; |
There was a problem hiding this comment.
Drop the stale imports before merge.
GRID_COLUMNS_MAP and css are unused after the refactor, and Biome is already failing noUnusedImports on this file.
Suggested cleanup
-import { type GridColumns, type GridProps, Grid, GRID_COLUMNS_MAP } from "@httpjpg/ui";
+import { type GridColumns, type GridProps, Grid } from "@httpjpg/ui";
import type { SbBlokData } from "@storyblok/react/rsc";
import { memo } from "react";
-import { css } from "styled-system/css";
import { mapSpacingToToken } from "../../lib/spacing-utils";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { type GridColumns, type GridProps, Grid, GRID_COLUMNS_MAP } from "@httpjpg/ui"; | |
| import type { SbBlokData } from "@storyblok/react/rsc"; | |
| import { memo } from "react"; | |
| import { css } from "styled-system/css"; | |
| import { type GridColumns, type GridProps, Grid } from "@httpjpg/ui"; | |
| import type { SbBlokData } from "@storyblok/react/rsc"; | |
| import { memo } from "react"; |
🧰 Tools
🪛 Biome (2.4.7)
[error] 4-4: Several of these imports are unused.
(lint/correctness/noUnusedImports)
[error] 7-7: This import is unused.
(lint/correctness/noUnusedImports)
🪛 GitHub Actions: CI
[error] 4-4: Biome lint (noUnusedImports): unused imports detected. Import includes 'Grid' and 'GRID_COLUMNS_MAP' from '@***/ui'.
[error] 7-7: Biome lint (noUnusedImports): unused import 'css' from 'styled-system/css'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/storyblok-ui/src/components/grid/SbGrid.tsx` around lines 4 - 7, The
file imports unused symbols GRID_COLUMNS_MAP and css which cause noUnusedImports
errors; remove GRID_COLUMNS_MAP and css from the import line so only the needed
symbols (Grid, type GridColumns, type GridProps) and the existing
SbBlokData/memo imports remain, ensuring the import statement in SbGrid.tsx is
cleaned up to match actual usage after the refactor.
Comment on lines
+27
to
+30
| spacingTop?: string; | ||
| spacingBottom?: string; | ||
| paddingTop?: string; | ||
| paddingBottom?: string; |
There was a problem hiding this comment.
Keep reading the legacy spacing keys until Storyblok data is migrated.
In packages/storyblok-sync/scripts/sync-components.ts, the schema switches to spacingTop / spacingBottom / paddingTop / paddingBottom, but existing stories still carry mt / mb / pt / pb. Dropping the old keys here makes those grids render without spacing immediately. Please keep a fallback until a one-time content migration has copied the old values forward.
Compatibility fallback
blok: {
_uid: string;
items?: SbBlokData[];
columns?: string;
columnsMd?: string;
columnsLg?: string;
gap?: string;
rowGap?: string;
columnGap?: string;
alignItems?: string;
justifyItems?: string;
justifyContent?: string;
autoFlow?: string;
boundingWidth?: "container" | "full";
isList?: boolean;
spacingTop?: string;
spacingBottom?: string;
paddingTop?: string;
paddingBottom?: string;
+ mt?: string;
+ mb?: string;
+ pt?: string;
+ pb?: string;
};
}
...
const {
items,
columns,
columnsMd,
columnsLg,
gap,
rowGap,
columnGap,
alignItems,
justifyItems,
justifyContent,
autoFlow,
boundingWidth = "full",
isList = false,
spacingTop,
spacingBottom,
paddingTop,
paddingBottom,
+ mt,
+ mb,
+ pt,
+ pb,
} = blok;
+
+ const resolvedSpacingTop = spacingTop ?? mt;
+ const resolvedSpacingBottom = spacingBottom ?? mb;
+ const resolvedPaddingTop = paddingTop ?? pt;
+ const resolvedPaddingBottom = paddingBottom ?? pb;
...
css: {
- mt: spacingTop ? mapSpacingToToken(spacingTop) : undefined,
- mb: spacingBottom ? mapSpacingToToken(spacingBottom) : undefined,
- pt: paddingTop ? mapSpacingToToken(paddingTop) : undefined,
- pb: paddingBottom ? mapSpacingToToken(paddingBottom) : undefined,
+ mt: resolvedSpacingTop ? mapSpacingToToken(resolvedSpacingTop) : undefined,
+ mb: resolvedSpacingBottom ? mapSpacingToToken(resolvedSpacingBottom) : undefined,
+ pt: resolvedPaddingTop ? mapSpacingToToken(resolvedPaddingTop) : undefined,
+ pb: resolvedPaddingBottom ? mapSpacingToToken(resolvedPaddingBottom) : undefined,
maxWidth: boundingWidth === "container" ? "breakpoint-xl" : undefined,
mx: boundingWidth === "container" ? "auto" : undefined,
},Also applies to: 102-105, 129-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/storyblok-ui/src/components/grid/SbGrid.tsx` around lines 27 - 30,
SbGrid is currently only reading the new spacing props (spacingTop,
spacingBottom, paddingTop, paddingBottom) which breaks render for existing
stories that still use legacy keys (mt, mb, pt, pb); update the prop handling in
the SbGrid component (and any related helpers used at the other spots
referenced) to prefer the new keys but fall back to the legacy keys when the new
ones are undefined (e.g., spacingTop ?? mt, spacingBottom ?? mb, paddingTop ??
pt, paddingBottom ?? pb), and ensure any style/class computation uses these
resolved values so both old and new stories render correctly until the content
migration runs.
Comment on lines
101
to
+123
| const showBlur = blurOnLoad && !highResLoaded && blurDataURL; | ||
| const showCopyrightInside = | ||
| copyright && | ||
| (copyrightPosition === "inline-white" || | ||
| copyrightPosition === "inline-black" || | ||
| copyrightPosition === "overlay"); | ||
| const showCopyrightInside = copyright && copyrightPosition !== "below"; | ||
|
|
||
| // Don't render if no src provided | ||
| if (!src) { | ||
| return null; | ||
| } | ||
| if (!src) return null; | ||
|
|
||
| return ( | ||
| const mediaContent = ( | ||
| <> | ||
| <Box | ||
| ref={(node: HTMLDivElement | null) => { | ||
| // Handle multiple refs | ||
| if (typeof ref === "function") { | ||
| ref(node); | ||
| } else if (ref) { | ||
| ref.current = node; | ||
| {showBlur && ( | ||
| <img | ||
| src={blurDataURL} | ||
| alt="" | ||
| aria-hidden="true" | ||
| className={css({ | ||
| position: "absolute", | ||
| inset: 0, | ||
| width: "100%", | ||
| height: "100%", | ||
| objectFit: "cover", | ||
| filter: "blur(20px)", | ||
| transform: "scale(1.1)", | ||
| transition: "opacity 0.3s ease-in-out", | ||
| })} | ||
| style={{ opacity: highResLoaded ? 0 : 1 }} |
There was a problem hiding this comment.
Don’t hide the main image when there is no blur placeholder.
When blurOnLoad is true but blurDataURL is missing, Lines 108-125 render no placeholder, but Line 148 still forces the main image to opacity: 0. That degrades to an empty box until the full image finishes loading.
Suggested fix
- const showBlur = blurOnLoad && !highResLoaded && blurDataURL;
+ const showBlur = blurOnLoad && !highResLoaded && Boolean(blurDataURL);
@@
- {showBlur && (
+ {showBlur && blurDataURL && (
<img
src={blurDataURL}
@@
- opacity: blurOnLoad && !highResLoaded ? 0 : 1,
+ opacity: showBlur ? 0 : 1,
}}Also applies to: 145-149
🧰 Tools
🪛 GitHub Actions: CI
[warning] 104-104: Biome lint (style/useBlockStatements): block statements are preferred in this position for if (!src) return null;.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/image/image.tsx` around lines 101 - 123, The main
image is forced to opacity 0 until load even when there is no blur placeholder;
update the main <img> rendering logic (the element that uses highResLoaded to
set style opacity) to base the initial hidden state on showBlur instead of just
highResLoaded—i.e., only set opacity to 0 while loading when showBlur is true,
otherwise render the main image fully opaque; apply the same change where the
main image opacity is handled around the other occurrence (lines 145–149) so
both places use showBlur || equivalent check (using blurOnLoad/blurDataURL via
showBlur).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description
This PR refactors the Video and Image components to use a new
AspectRatiowrapper component, eliminating code duplication and improving maintainability. The changes consolidate aspect ratio handling logic into a reusable component while simplifying the media component implementations.Key Changes:
UI Components:
AspectRatiowrapper; consolidated YouTube and Vimeo embed logic; simplified copyright label renderingAspectRatiowrapper for aspect ratio support; refactored to use conditional rendering based on aspect ratio presenceStoryblok Components:
SbMediaWrappercomponent; inlined spacing and width mapping directly into components usingBoxSbMediaWrappercomponent (no longer needed with inlined logic)Benefits:
wrapperStyleandwrapperClassNameprops🎯 Type of Change
🧪 Testing
✅ Checklist
📦 Packages/Apps Affected
@httpjpg/ui@httpjpg/storyblok-ui📚 Additional Context
This refactoring improves the codebase by leveraging the
AspectRatiocomponent that was already available but underutilized. The removal ofwrapperStyleandwrapperClassNameprops simplifies the API surface while theAspectRatiocomponent handles all aspect ratio concerns consistently.Breaking Changes:
Videocomponent:wrapperStyleandwrapperClassNameprops removedImagecomponent:wrapperStyleandwrapperClassNameprops removedSbMediaWrappercomponent: Removed from exports (functionality inlined into consuming components)https://claude.ai/code/session_01DLAMCPXYx7Z9kuqQ5RKLNM
Summary by CodeRabbit