fix(#298): align catalogue card rows, keeping subtitle space conditional - #302
Merged
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ 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 e reagendo agli aggiornamenti AJAX. Una spec Playwright verifica i casi desktop, mobile e multi-griglia. ChangesAllineamento delle card catalogo
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Catalogo
participant BooksGrid
participant ScriptAllineamento
participant Card
Catalogo->>BooksGrid: aggiorna le card via AJAX
BooksGrid->>ScriptAllineamento: dispatch pinakes:catalog-grid-updated
ScriptAllineamento->>Card: raggruppa per riga e misura le altezze
ScriptAllineamento->>Card: applica altezze e placeholder subtitle-ph
Card-->>BooksGrid: restituisce il layout riallineato
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Owner
Author
|
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). |
Five review findings on the alignment script/test: 1. The script measured heights on DOMContentLoaded/load only, never on document.fonts.ready — a late web-font swap could reflow a title from one to two lines after the pass, clipping it (line-clamp + fixed height) and leaving the row misaligned until a resize. Now re-aligns on fonts.ready. 2. Layout thrashing: alignGrid() interleaved reads (getBoundingClientRect / offsetHeight) and writes (height) row by row, forcing repeated synchronous reflows, and ran twice on initial load. Split into a single READ phase then a single WRITE phase, and coalesced all triggers (load, resize, AJAX, fonts) through one requestAnimationFrame. 3. equalise wrote height:0px when every card measured 0 (grid inside a display:none ancestor), collapsing titles. Now skips a row whose titles measure 0 (not laid out). 4. Added a mobile single-column test (375px): asserts one column, zero spurious placeholders, and no zero-height titles — a path the suite never exercised. 5. Test 1 now waits for document.fonts.ready before measuring (was a fixed 600ms that could sample a pre-font-swap layout) and pins its assertions to the seeded fixture so ambient data can't silently substitute unrelated books. Verified: all 4 E2E tests green (alignment unchanged for the asserted cases).
Owner
Author
|
Self-reviewed the alignment script before release. All findings P2; fixed in 338387b:
All 4 E2E tests green; alignment behaviour unchanged for the asserted cases. Cross-checked against #303: disjoint regions in the catalogue views, no semantic conflict. |
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.
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