Merged
Conversation
Co-authored-by: luandev <6452989+luandev@users.noreply.github.com>
…ibility Co-authored-by: luandev <6452989+luandev@users.noreply.github.com>
Co-authored-by: luandev <6452989+luandev@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix cropping issue for cover images in library view
Fix cover image cropping in library view
Jan 8, 2026
Owner
|
@copilot e2e seem not to be working |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes cover image cropping issues in the library view by changing the CSS object-fit property from cover to contain, preventing content loss. The PR also adds E2E tests to validate the fix across Browse and Library views.
Key changes:
- Changed cover image rendering strategy to preserve full image content
- Added background color to fill gaps when aspect ratios differ
- Introduced E2E tests for cover image CSS validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
apps/web/src/styles.css |
Changed .cover-img from object-fit: cover to contain and added background color |
tests/e2e/cover-image.spec.ts |
New E2E test validating object-fit property and dimensions across Browse/Library pages |
| await test.step('Browse page - Verify cover image CSS', async () => { | ||
| // Should be on browse page by default, but navigate to be sure | ||
| await page.click('nav a[href="/browse"]'); | ||
| await expect(page.getByRole('heading', { name: 'Browse CrocDB' })).toBeVisible(); |
There was a problem hiding this comment.
The heading text should be 'Browse Crocdb' (lowercase 'db'), not 'Browse CrocDB'. This will cause the test to fail as it won't find the expected heading. The actual heading in BrowsePage.tsx is 'Browse Crocdb'.
Suggested change
| await expect(page.getByRole('heading', { name: 'Browse CrocDB' })).toBeVisible(); | |
| await expect(page.getByRole('heading', { name: 'Browse Crocdb' })).toBeVisible(); |
Comment on lines
+31
to
+73
| // Navigate to Browse page to check cover images there | ||
| await test.step('Browse page - Verify cover image CSS', async () => { | ||
| // Should be on browse page by default, but navigate to be sure | ||
| await page.click('nav a[href="/browse"]'); | ||
| await expect(page.getByRole('heading', { name: 'Browse CrocDB' })).toBeVisible(); | ||
|
|
||
| // Perform a search to get some results | ||
| await page.fill('input[name="search"]', 'Super Mario'); | ||
| await page.click('button[type="submit"]'); | ||
|
|
||
| // Wait for search results (may fail if API is unavailable) | ||
| await page.waitForTimeout(2000); | ||
|
|
||
| // Check if any cover images are present | ||
| const coverImages = page.locator('.cover-img'); | ||
| const coverCount = await coverImages.count(); | ||
|
|
||
| if (coverCount > 0) { | ||
| // Get the first cover image | ||
| const firstCover = coverImages.first(); | ||
|
|
||
| // Verify it's visible | ||
| await expect(firstCover).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| // Check computed style for object-fit | ||
| const objectFit = await firstCover.evaluate((el) => { | ||
| return window.getComputedStyle(el).objectFit; | ||
| }); | ||
|
|
||
| // Should be 'contain' not 'cover' | ||
| expect(objectFit).toBe('contain'); | ||
|
|
||
| // Verify the image has the expected dimensions | ||
| const dimensions = await firstCover.boundingBox(); | ||
| if (dimensions) { | ||
| // Cover dimensions from theme.ts: --cover-w: 200px, --cover-h: 266px | ||
| // Allow tolerance for rendering differences | ||
| expect(Math.abs(dimensions.width - COVER_WIDTH)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | ||
| expect(Math.abs(dimensions.height - COVER_HEIGHT)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | ||
| } | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test performs an unnecessary search operation to verify CSS properties. The object-fit style can be checked on any cover image element without requiring search results. This makes the test more fragile (dependent on external API availability) and slower. Consider simplifying to just check the CSS on existing elements in the library view, or use a mock/fixture approach if cover images need to be present.
Suggested change
| // Navigate to Browse page to check cover images there | |
| await test.step('Browse page - Verify cover image CSS', async () => { | |
| // Should be on browse page by default, but navigate to be sure | |
| await page.click('nav a[href="/browse"]'); | |
| await expect(page.getByRole('heading', { name: 'Browse CrocDB' })).toBeVisible(); | |
| // Perform a search to get some results | |
| await page.fill('input[name="search"]', 'Super Mario'); | |
| await page.click('button[type="submit"]'); | |
| // Wait for search results (may fail if API is unavailable) | |
| await page.waitForTimeout(2000); | |
| // Check if any cover images are present | |
| const coverImages = page.locator('.cover-img'); | |
| const coverCount = await coverImages.count(); | |
| if (coverCount > 0) { | |
| // Get the first cover image | |
| const firstCover = coverImages.first(); | |
| // Verify it's visible | |
| await expect(firstCover).toBeVisible({ timeout: 5000 }); | |
| // Check computed style for object-fit | |
| const objectFit = await firstCover.evaluate((el) => { | |
| return window.getComputedStyle(el).objectFit; | |
| }); | |
| // Should be 'contain' not 'cover' | |
| expect(objectFit).toBe('contain'); | |
| // Verify the image has the expected dimensions | |
| const dimensions = await firstCover.boundingBox(); | |
| if (dimensions) { | |
| // Cover dimensions from theme.ts: --cover-w: 200px, --cover-h: 266px | |
| // Allow tolerance for rendering differences | |
| expect(Math.abs(dimensions.width - COVER_WIDTH)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | |
| expect(Math.abs(dimensions.height - COVER_HEIGHT)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | |
| } | |
| } | |
| }); | |
Comment on lines
+100
to
+106
| // Verify background color is set (to fill empty space) | ||
| const backgroundColor = await firstLibraryCover.evaluate((el) => { | ||
| return window.getComputedStyle(el).backgroundColor; | ||
| }); | ||
|
|
||
| // Should have a background color (not transparent) | ||
| expect(backgroundColor).not.toBe('rgba(0, 0, 0, 0)'); |
There was a problem hiding this comment.
The background color assertion is too strict. When using object-fit: contain, a transparent background (rgba(0, 0, 0, 0)) is technically valid and doesn't indicate a problem. The background property in the CSS serves as a fallback for letterboxing, but browsers may not always compute it as non-transparent depending on the rendering context. This assertion could cause false test failures. Consider either removing this check or making it less strict (e.g., just verify the property is defined, not its computed value).
Suggested change
| // Verify background color is set (to fill empty space) | |
| const backgroundColor = await firstLibraryCover.evaluate((el) => { | |
| return window.getComputedStyle(el).backgroundColor; | |
| }); | |
| // Should have a background color (not transparent) | |
| expect(backgroundColor).not.toBe('rgba(0, 0, 0, 0)'); | |
| // Verify background color property is set (actual value may be transparent) | |
| const backgroundColor = await firstLibraryCover.evaluate((el) => { | |
| return window.getComputedStyle(el).backgroundColor; | |
| }); | |
| // Should have a defined backgroundColor value (avoid over-constraining exact color/opacity) | |
| expect(backgroundColor).toBeTruthy(); |
Comment on lines
+111
to
+143
| test('cover image placeholder should display correctly', async ({ page }) => { | ||
| await page.goto('/'); | ||
|
|
||
| // Dismiss welcome view if present | ||
| const welcomeSkipButton = page.getByRole('button', { name: /Skip|Get Started/i }); | ||
| try { | ||
| await welcomeSkipButton.waitFor({ state: 'visible', timeout: 2000 }); | ||
| await welcomeSkipButton.click(); | ||
| await page.waitForTimeout(500); | ||
| } catch { | ||
| // Welcome view not shown | ||
| } | ||
|
|
||
| // Navigate to library | ||
| await page.click('nav a[href="/library"]'); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Check for placeholder elements | ||
| const placeholders = page.locator('.thumb-placeholder'); | ||
| const placeholderCount = await placeholders.count(); | ||
|
|
||
| if (placeholderCount > 0) { | ||
| const firstPlaceholder = placeholders.first(); | ||
| await expect(firstPlaceholder).toBeVisible(); | ||
|
|
||
| // Verify placeholder has correct dimensions | ||
| const dimensions = await firstPlaceholder.boundingBox(); | ||
| if (dimensions) { | ||
| expect(Math.abs(dimensions.width - COVER_WIDTH)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | ||
| expect(Math.abs(dimensions.height - COVER_HEIGHT)).toBeLessThanOrEqual(DIMENSION_TOLERANCE); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test checks placeholder elements (.thumb-placeholder), which are div elements that don't use the .cover-img class or object-fit property. The test name and parent describe block suggest this is about cover image rendering with object-fit: contain, but placeholders are not actual images and don't use object-fit. This test doesn't validate the fix for cover image cropping. Consider either removing this test or clarifying its purpose and moving it to a separate test suite for placeholder rendering.
Co-authored-by: luandev <6452989+luandev@users.noreply.github.com>
Contributor
Author
Fixed in commit 2971f22. The issue was that the test was trying to navigate to Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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
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
object-fit: covertoobject-fit: containto preserve full image within 200×266px container.Before/After:
CSS Changes (
apps/web/src/styles.css):E2E Tests (
tests/e2e/cover-image.spec.ts):object-fit: containis applied on Browse and Library pages/not/browse)Testing
Checklist
/semver: patch,/semver: minor, or/semver: major. (See template for examples)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.