fix(#298): align catalogue card rows, keeping subtitle space conditional - #302
fix(#298): align catalogue card rows, keeping subtitle space conditional#302fabiodalez-dev wants to merge 5 commits into
Conversation
…e-less rows The book title had a fixed min-height and the subtitle was rendered only when present, so cards with a subtitle pushed author/publisher/"Details" down out of line with their neighbours, while cards with a short title left a gap. #298 proposed removing the mobile min-height, but that only makes the title inherit the 2.8em base (taller, not shorter) and doesn't address alignment. Instead: keep the subtitle conditional (no reserved space on the majority of rows that have none) and equalise per grid row with a small layout script — it sets every title in a row to the row's tallest title, pads the shorter real subtitles, and injects a hidden .book-subtitle placeholder on the cards of a subtitle row that have none. Result: rows with no subtitle stay compact; rows that contain one line up title → subtitle → author → publisher → Details across all their cards. Pure layout (heights only) — colours stay on the theme variables, so it's identical across themes. tests/catalog-subtitle-align-298.spec.js seeds a catalogue with plain and subtitled books and asserts, per row: subtitle rows keep authors aligned, a placeholder is reserved on their plain cards, and subtitle-less rows get none.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLa griglia catalogo usa troncamenti CSS per titoli, autori e metadati. Uno script equalizza titolo e sottotitolo per riga, creando placeholder nascosti quando necessario. Una nuova spec Playwright verifica l’allineamento con dati aventi sottotitoli presenti e assenti. ChangesAllineamento delle card catalogo
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/frontend/catalog-grid.php (1)
200-211: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllinea
line-heightaheightin.book-author.
height: 1.3emcon font-size fixed funziona solo se la riga è calcolata su1.3; senzaline-heightesplicito il testo può essere tagliato sopra/sotto dal box conoverflow: hidden. Imposta lo stesso valore coerente tra font-size, height e line-height.🛠️ Fix proposto
.book-author { font-size: 0.875rem; color: var(--text-secondary); margin-bottom: 0.5rem; /* `#298`: fixed single-line box so a long author name can't push the row below out of alignment with adjacent cards. */ height: 1.3em; + line-height: 1.3; display: -webkit-box; -webkit-line-clamp: 1; -webkit-box-orient: vertical; overflow: hidden; }🤖 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 `@app/Views/frontend/catalog-grid.php` around lines 200 - 211, Update the .book-author rule to define an explicit line-height matching its 1.3em height, keeping the font-size and single-line clamping behavior aligned so the text is not vertically clipped.
🤖 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.
Inline comments:
In `@app/Views/frontend/catalog-grid.php`:
- Around line 324-357: Limit row grouping to each individual .books-grid
container instead of combining all .book-card elements globally. Update align()
to iterate over every .books-grid and call rowsOf() only with that grid’s cards,
preserving separate title/subtitle sizing and placeholder creation for each
grid.
---
Outside diff comments:
In `@app/Views/frontend/catalog-grid.php`:
- Around line 200-211: Update the .book-author rule to define an explicit
line-height matching its 1.3em height, keeping the font-size and single-line
clamping behavior aligned so the text is not vertically clipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01d512c1-730b-4b39-a352-22f7a36e1f2e
📒 Files selected for processing (2)
app/Views/frontend/catalog-grid.phptests/catalog-subtitle-align-298.spec.js
CodeRabbit: align() grouped every .book-card on the page by vertical position, so if a page renders two .books-grid containers, cards from different grids at the same top could be merged into one logical row and get wrong placeholders / heights. Iterate over each .books-grid and group only its own cards.
…ight CodeRabbit: .book-author has height:1.3em + overflow:hidden but no explicit line-height, so the default (~1.2) could clip the single line top/bottom. Set line-height:1.3 to match the box height (.book-meta already pairs 1.5/1.5em).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/frontend/catalog-grid.php (1)
38-38: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUsare
JSON_HEX_TAGnella serializzazione inline.
$defaultCoverUrlviene inserito nell’handler JavaScript senzaJSON_HEX_TAG. Anche se oggi il valore deriva da un path fisso, la view viola il requisito di sicurezza perapp/Views/**e resta fragile se il valore diventa configurabile.Proposta
- onerror="this.onerror=null;this.src=<?= htmlspecialchars(json_encode($defaultCoverUrl), ENT_QUOTES, 'UTF-8') ?>"> + onerror="this.onerror=null;this.src=<?= htmlspecialchars(json_encode($defaultCoverUrl, JSON_HEX_TAG), ENT_QUOTES, 'UTF-8') ?>">As per path instructions, le view devono usare
json_encode(..., JSON_HEX_TAG)per ogni dato PHP inserito in JavaScript.🤖 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 `@app/Views/frontend/catalog-grid.php` at line 38, Aggiorna la serializzazione di $defaultCoverUrl nell’attributo onerror della view per includere JSON_HEX_TAG insieme alle opzioni esistenti di json_encode. Mantieni invariati htmlspecialchars e il comportamento di fallback dell’immagine, assicurando che ogni dato PHP inserito in JavaScript usi la codifica richiesta.Source: Path instructions
🤖 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.
Inline comments:
In `@app/Views/frontend/catalog-grid.php`:
- Around line 382-386: Aggiungere una regressione in
tests/catalog-subtitle-align-298.spec.js che renderizzi due elementi .books-grid
con card alla stessa coordinata verticale, quindi verifichi separatamente
l’allineamento delle card all’interno di ciascuna griglia. Evitare raccolte o
asserzioni globali: usare ciascun .books-grid come ambito indipendente per
confermare che le righe non vengano unite tra griglie.
---
Outside diff comments:
In `@app/Views/frontend/catalog-grid.php`:
- Line 38: Aggiorna la serializzazione di $defaultCoverUrl nell’attributo
onerror della view per includere JSON_HEX_TAG insieme alle opzioni esistenti di
json_encode. Mantieni invariati htmlspecialchars e il comportamento di fallback
dell’immagine, assicurando che ogni dato PHP inserito in JavaScript usi la
codifica richiesta.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 953f64f8-7bda-4932-8363-380ae4ce0774
📒 Files selected for processing (1)
app/Views/frontend/catalog-grid.php
CodeRabbit: the spec only exercised a single grid, so a return to global row grouping wouldn't be caught. Add a case that builds two side-by-side grids sharing the same row top, and asserts a subtitle in grid A reserves space only within grid A — grid B (no subtitle) is untouched.
|
Fixed the post-AJAX alignment gap in a7810c0. The catalogue now emits a dedicated event after replacing grid HTML; the alignment handler reruns for filter/sort/pagination refreshes. Added a real Playwright regression that changes sorting through AJAX and verifies replacement-card alignment (targeted spec: 3/3 pass). |
1 participant
Builds on @Himura2la's #298. The underlying observation is right — the catalogue cards don't line up cleanly — but the proposed change (removing the mobile
min-heighton.book-title) doesn't fix it: with the2.8embase rule still global, removing the2.4emmobile override just makes the title inherit the base (taller, not shorter), and it doesn't touch what actually misaligns the rows.What actually misaligns the cards
.book-titlehad a fixedmin-heightof two lines: a 3-line title overflows it and a 1-line title leaves a gap, so author/publisher drift between cards..book-author/.book-metareserve their space with a hidden placeholder; the subtitle didn't). A card with a subtitle pushes everything below it down relative to neighbours without one.On a real catalogue only a small fraction of books have a subtitle, so reserving that space on every card (my first attempt) wastes it on most rows.
Fix — conditional space, aligned per row
Keep the subtitle conditional, and equalise per grid row with a small layout script:
.book-subtitleplaceholder (same margins) of that height.So a row with no subtitle stays compact, and a row that contains one lines up title → subtitle → author → publisher → "Details" across all its cards. It recomputes on resize. Pure layout — only heights are touched; colours stay on the theme variables, so it's identical across every theme and in dark mode.
Tests
tests/catalog-subtitle-align-298.spec.jsseeds a catalogue with plain and subtitled books and asserts, per row: subtitle rows keep authors aligned across cards, a placeholder is reserved on their plain cards, and subtitle-less rows get none. (This test caught a real bug during development: two subtitles of different length in the same row — now fixed by equalising the real subtitles too.)Full-tree PHPStan level 5 clean.
Summary by CodeRabbit
Miglioramenti
Test