-
Notifications
You must be signed in to change notification settings - Fork 68
Fix asset-info overlap #531
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?
Fix asset-info overlap #531
Conversation
WalkthroughIntroduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/elements/asset-info.svelte (1)
81-81: Apply conditional truncation to formattedDate for consistency in split viewThe
formattedDateat line 81 lacks the conditionalshort-text={split}class applied to all other metadata fields (lines 87, 93, 99, 105). Since photoDateFormat is configurable and can produce varying lengths depending on user settings, add the same truncation class for consistency and to prevent potential text overlap in split view:<span class="info-text" class:short-text={split}>{formattedDate}</span>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
immichFrame.Web/src/lib/components/elements/asset-info.svelte(4 hunks)immichFrame.Web/src/lib/components/elements/image-component.svelte(3 hunks)immichFrame.Web/src/lib/components/elements/image.svelte(3 hunks)
🔇 Additional comments (5)
immichFrame.Web/src/lib/components/elements/asset-info.svelte (2)
17-17: LGTM! Split prop added correctly.The
splitprop is properly typed and destructured for use in conditional styling.Also applies to: 28-28
118-126: No CSS property fix needed. The code is correct.The property
text-wrap: nowrapis a valid CSS keyword value, contrary to the original review. The combination oftext-wrap: nowrap,overflow: hidden, andtext-overflow: ellipsisis a valid pattern for text truncation with ellipsis and requires no changes.Regarding the max-width value discrepancy (40vw vs 44vw claimed in the PR description): the code currently uses
max-width: 40vwat line 119. Please verify the PR description and update it if it incorrectly states 44vw, or clarify if the value should be changed to match documentation.immichFrame.Web/src/lib/components/elements/image.svelte (1)
23-23: LGTM! Split prop threaded correctly.The
splitprop is properly added to the interface, destructured, and forwarded to theAssetInfocomponent.Also applies to: 38-38, 184-184
immichFrame.Web/src/lib/components/elements/image-component.svelte (2)
19-19: LGTM! Split prop initialized with sensible default.The optional
splitprop with afalsedefault value is a good pattern, ensuring backward compatibility.Also applies to: 37-37
95-95: LGTM! Split prop forwarded consistently.The
splitprop is correctly passed to all threeImagecomponent instances (both split-view images and the default single-image view).Also applies to: 111-111, 129-129
Summary
Fixes #434: Add responsive max-width constraints to asset info containers
Changes
44vw22vwwhen two images are displayed side-by-sideTesting
The fix works reliably across most screen sizes, including desktops, tablets, and phones.
Only very narrow split-view scenarios still show some overlap, which would require moving either the clock or asset info to fully resolve.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.