Responsive image layout 080526#2360
Closed
simo6529 wants to merge 5 commits into
Closed
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements responsive image grid layout for drops and waves by adding responsive image scaling helpers, refactoring FallbackImage for loader support, forwarding responsive-image props through the component hierarchy, and enabling a CSS grid for image-only media with expanded tests. ChangesResponsive Image Grid for Drops/Waves
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4bfea79cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/waves/drops/WaveDropPartContentMedias.tsx (1)
66-67: 💤 Low valueConsider adjusting
sizesfor the 2-image case.When
activePart.media.length === 2, the grid usesmd:tw-grid-cols-2(50% width per image), butRESPONSIVE_IMAGE_GRID_SIZESalways uses33vwon desktop. This may cause the browser to request smaller images than needed for the 2-column layout.💡 Suggested improvement
+const RESPONSIVE_IMAGE_GRID_SIZES_2_COL = "(max-width: 767px) 100vw, 50vw"; +const RESPONSIVE_IMAGE_GRID_SIZES_3_COL = "(max-width: 767px) 100vw, 33vw"; // In the component: +const responsiveImageSizes = + activePart.media.length === 2 + ? RESPONSIVE_IMAGE_GRID_SIZES_2_COL + : RESPONSIVE_IMAGE_GRID_SIZES_3_COL; // Then use responsiveImageSizes instead of RESPONSIVE_IMAGE_GRID_SIZESAlso applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/waves/drops/WaveDropPartContentMedias.tsx` around lines 66 - 67, The sizes used for responsive images aren't updated for the 2-image grid: when activePart.media.length === 2 the component sets desktopGridColumnClassName to "md:tw-grid-cols-2" but RESPONSIVE_IMAGE_GRID_SIZES (used to set the image sizes attribute) still returns desktop widths based on a 3-column layout (33vw). Update the logic in WaveDropPartContentMedias to detect the 2-image case and return/assign a different sizes string for the md breakpoint (e.g., ~50vw per image) whenever activePart.media.length === 2; modify both the desktopGridColumnClassName/RESPONSIVE_IMAGE_GRID_SIZES usage sites (including the other occurrence around the second image-rendering block) so the <img> sizes attribute matches the actual md grid columns.__tests__/components/DropListItemContentMediaImage.test.tsx (1)
15-47: ⚡ Quick winCover the
sizesforwarding path in the responsive-image test.Right now this only proves that responsive mode turns on
optimizeand passes a loader. IfDropListItemContentMediaImagestops forwarding thesizeshint, this suite will still pass even though the browser would pick the wrong candidates. Exposingsizeson theFallbackImagemock and asserting it here would close that gap.🧪 Suggested test tightening
( { alt, primarySrc, fallbackSrc, optimize, loader, + sizes, onClick, onError, onLoad, }: any, ref: any ) => ( <img ref={ref} alt={alt} src={primarySrc} data-fallback-src={fallbackSrc} data-optimize={optimize === undefined ? "" : String(optimize)} data-has-loader={loader ? "true" : "false"} + data-sizes={sizes ?? ""} onClick={onClick} onError={onError} onLoad={onLoad} /> ) @@ it("uses the responsive loader only when responsive srcset mode is enabled", () => { render( - <DropListItemContentMediaImage src="img" useResponsiveImageSrcSet /> + <DropListItemContentMediaImage + src="img" + useResponsiveImageSrcSet + imageSizes="100vw" + /> ); const img = screen.getByAltText("Drop media"); expect(img).toHaveAttribute("data-optimize", "true"); expect(img).toHaveAttribute("data-has-loader", "true"); + expect(img).toHaveAttribute("data-sizes", "100vw"); });Also applies to: 101-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/components/DropListItemContentMediaImage.test.tsx` around lines 15 - 47, The test's FallbackImage mock in DropListItemContentMediaImage.test.tsx doesn't expose the sizes prop, so add sizes to the mocked component's props (alongside alt, primarySrc, fallbackSrc, optimize, loader, onClick, onError, onLoad) and render it as an attribute (e.g., data-sizes={sizes}) in the returned <img>; then update the responsive-image test to assert that DropListItemContentMediaImage forwards the expected sizes hint to the FallbackImage mock (check data-sizes value) to cover the sizes forwarding path for responsive mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/components/DropListItemContentMediaImage.test.tsx`:
- Around line 15-47: The test's FallbackImage mock in
DropListItemContentMediaImage.test.tsx doesn't expose the sizes prop, so add
sizes to the mocked component's props (alongside alt, primarySrc, fallbackSrc,
optimize, loader, onClick, onError, onLoad) and render it as an attribute (e.g.,
data-sizes={sizes}) in the returned <img>; then update the responsive-image test
to assert that DropListItemContentMediaImage forwards the expected sizes hint to
the FallbackImage mock (check data-sizes value) to cover the sizes forwarding
path for responsive mode.
In `@components/waves/drops/WaveDropPartContentMedias.tsx`:
- Around line 66-67: The sizes used for responsive images aren't updated for the
2-image grid: when activePart.media.length === 2 the component sets
desktopGridColumnClassName to "md:tw-grid-cols-2" but
RESPONSIVE_IMAGE_GRID_SIZES (used to set the image sizes attribute) still
returns desktop widths based on a 3-column layout (33vw). Update the logic in
WaveDropPartContentMedias to detect the 2-image case and return/assign a
different sizes string for the md breakpoint (e.g., ~50vw per image) whenever
activePart.media.length === 2; modify both the
desktopGridColumnClassName/RESPONSIVE_IMAGE_GRID_SIZES usage sites (including
the other occurrence around the second image-rendering block) so the <img> sizes
attribute matches the actual md grid columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63a884f2-a8f5-4565-ac22-0bd13d95f2a4
📒 Files selected for processing (17)
__tests__/components/DropListItemContentMediaImage.test.tsx__tests__/components/common/FallbackImage.test.tsx__tests__/components/drops/view/item/content/media/DropListItemContentMedia.test.tsx__tests__/components/waves/drops/WaveDrop.test.tsx__tests__/components/waves/drops/WaveDropPartContent.test.tsx__tests__/components/waves/drops/WaveDropPartContentMedias.test.tsx__tests__/helpers/image.helpers.test.tscomponents/common/FallbackImage.tsxcomponents/drops/view/item/content/media/DropListItemContentMedia.tsxcomponents/drops/view/item/content/media/DropListItemContentMediaImage.tsxcomponents/waves/drops/WaveDrop.tsxcomponents/waves/drops/WaveDropContent.tsxcomponents/waves/drops/WaveDropPart.tsxcomponents/waves/drops/WaveDropPartContent.tsxcomponents/waves/drops/WaveDropPartContentMedias.tsxcomponents/waves/drops/WaveDropPartDrop.tsxhelpers/image.helpers.ts
|
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
1 participant
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.
Summary by CodeRabbit
New Features
Refactor
Tests