Configurable event image layout (closes #137)#139
Open
fabiodalez-dev wants to merge 5 commits into
Open
Conversation
Issue #137 (HansUwe52): the featured image on the public event detail page renders at 100% container width with no max-height constraint, so sproportioned uploads (e.g. a 1791x927 book cover used as an event image) dominate the viewport. Adds a CMS-tab setting 'cms.event_image_layout' with four options: - contained (default) max-height: clamp(280px, 50vw, 480px) + object-fit: contain. Solves the reported case while preserving the previous look for reasonably-sized images. - banner aspect-ratio: 16/9 + object-fit: cover. Crops to a uniform hero banner. - full Legacy 100%-width-no-constraint behaviour, for users who depended on it. - thumb Right-floated 3:4 portrait thumbnail that wraps the event body text. Collapses to full-width on viewports under 640px. The setting lives in the existing system_settings KV store (no schema migration needed) and is wired through the existing pattern used by 'updateLabels' / 'updateSharingSettings': - SettingsController::updateEventSettings (validated allow-list) - resolveEventSettings() exposed to the settings view - POST /admin/settings/events route protected by CsrfMiddleware + AdminAuthMiddleware - select rendered inside the existing 'Eventi' card on the CMS tab, so the admin doesn't have to discover a new tab FrontendController::event() reads and re-validates the setting against the same allow-list before passing it to the view; event-detail.php narrows once more before composing the CSS class, so a future controller refactor cannot leak an unknown layout into the DOM. i18n: 8 new keys added to all four locale files (it_IT/en_US/de_DE/fr_FR) in the same commit, per project policy. Tests: tests/issue-137-event-image-layout.spec.js — 5 cases covering the four explicit layouts plus the default fallback. Each test mutates cms.event_image_layout in the KV store directly, requests the public event page, and asserts the figure carries the matching CSS modifier and data-event-cover-layout attribute. All 5 pass. PHPStan: clean (no new findings; pre-existing storage/plugins issues unchanged at 11).
Self-review feedback on the initial layout palette:
- 'contained' was using an arbitrary max-height: 480px that produced
uneven silhouettes depending on the source aspect ratio. Replaced
with aspect-ratio: 3/2 + object-fit: cover so the cover always
lands on a predictable photographic crop.
- 'thumb' was using CSS float: right, which caused the figure to
escape the parent .event-card whenever the body content was short
(e.g. a one-paragraph event) — visually the image then overlapped
the page footer. Refactored to a CSS grid layout driven by a new
.event-card--thumb-layout modifier on the parent article: the
figure now lives in its own column/row and is geometrically
constrained inside the card. On viewports below 768px the grid
collapses to a vertical stack.
- Bonus on desktop: the side-thumbnail sticks while the reader
scrolls long event descriptions (position: sticky).
UI labels updated to match the new design ('Foto 3:2' instead of
'Adattato (max 480px)') in all four locale files.
Tests: added a 6th case to tests/issue-137-event-image-layout.spec.js
that creates a very short event content and asserts
figure.bottom <= card.bottom at desktop width — the exact invariant
the float bug used to violate. All 6 cases pass.
Previous iterations all rendered at 100% container width and only
differed by aspect-ratio, so the original complaint (a 1791x927 image
dominating the event page) was still visible in three of four presets.
The whole point of the setting is to LIMIT the image footprint.
Resized accordingly:
full 100% x auto legacy enormous (opt-in)
banner 100% x 220px max low decorative strip
contained 420px max, centred small poster (DEFAULT, solves
the reported case directly)
thumb 240px wide side thumbnail next to body text
Source aspect ratio is preserved on contained/thumb-mobile — shrinking
the footprint is the goal, forcing crops would just trade one issue
for another.
UI labels rewritten so the size is explicit ('Piccola centrata
(max 420px)', 'Miniatura affiancata al testo (240px)', etc.) and
the librarian can pick at a glance. All 4 locale files updated.
Tests: added a measurement-based regression that asserts each preset
renders at a distinct, capped size:
contained.width <= 420px AND < 70% of card.width
thumb.width <= 245px
banner.height <= 225px
full.width > 85% of card.width
Catches the whole class of regression where four CSS modifiers exist
but render identically. All 7 tests pass.
User feedback: small images should sit on the left, not centred. - contained: margin-left: 0 (was: margin-left/right: auto) - thumb on mobile: margin: 0 0 1.5rem 0 (was: 0 auto 1.5rem auto) Small images centred in a wide card read as a 'centred hero' which is the same visual problem we are trying to escape: the image dominates its own row. Left-aligned, the image becomes part of the body flow. UI label updated: 'Piccola a sinistra (max 420px)' replaces 'Piccola centrata (max 420px)'. All four locale files updated. Test guard added: the effective-size test now also asserts that figure.x is within 100px of card.x (i.e. only inner padding) for both 'contained' and 'thumb'. Catches any future refactor that reintroduces margin: auto. All 7 cases pass.
The edit form has separate controls: a 'remove_image' checkbox and a
'featured_image' file input. When the user ticks both (replace the
old image AND clear the slot in one submit), the existing if/elseif
processed 'remove' first and skipped the upload branch — the new
file was discarded and featured_image ended up NULL.
Fix:
- Invert the priority: handle the file upload first; only run the
remove path when no upload was submitted. This matches user
intent (a new file always means 'replace').
- Look up the previous featured_image before the UPDATE and unlink
the orphan file from disk after the row is updated. Path-
traversal-safe via realpath() + base-directory containment, the
same contract handleImageUpload() upholds when writing.
Test: tests/issue-137-event-image-layout.spec.js — added an
end-to-end case that drives the actual admin form (login, edit
event, tick the remove checkbox, attach a new file, submit) and
asserts the DB row points at a NEW upload path (regex matches
handleImageUpload()'s naming scheme) — not NULL, not the old
sentinel. Catches the regression at the controller layer where the
historic bug lived. All 8 cases pass.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Reset content to a long enough body so 'banner' / 'contained' | ||
| // have something below them to measure against. | ||
| const longContent = '<p>' + 'Test event description. '.repeat(40) + '</p>'; | ||
| const sqlSafeLong = longContent.replace(/'/g, "\\'"); |
|
|
||
| // Shrink the event content so a CSS float would expose the bug. | ||
| const shortContent = '<p>Breve.</p>'; | ||
| const sqlSafe = shortContent.replace(/'/g, "\\'"); |
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
2 participants
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
Closes #137.
Adds an admin-controlled layout setting for the featured image on the public event detail page (
/eventi/<slug>). The previous CSS rendered every uploaded image at 100% container width with no height constraint, so a 1791×927 cover (the case @HansUwe52 reported) dominated the viewport.A new setting
cms.event_image_layoutlives in the existing CMS settings tab. Four presets, in increasing visual footprint:All four labels are translated in
it_IT,en_US,de_DE,fr_FRin the same commit (per i18n policy).Architecture
system_settings(category=cms, key=event_image_layout). No migration needed.POST /admin/settings/events(CSRF + AdminAuth middleware), pattern identical toupdateLabels/updateSharingSettings.Bonus fix in the same branch
While reviewing the events admin form a separate bug was found (last commit): when the admin submits the edit form with BOTH a new
featured_imageupload ANDremove_image=1checked, the previousif/elseifevaluatedremove_imagefirst and silently discarded the upload. The branch flips the priority (upload wins) and adds best-effort cleanup of the orphaned file frompublic/uploads/events/, path-traversal-safe.Test plan
8 cases in
tests/issue-137-event-image-layout.spec.js, all passing:cms.event_image_layoutis unset, the page rendersevent-cover--containedfull→event-cover--fullclass +data-event-cover-layout='full'banner→event-cover--bannercontained→event-cover--containedthumb→event-cover--thumbcontained.width ≤ 420px AND < 70% card.width,thumb.width ≤ 245px,banner.height ≤ 225px,full.width > 85% card.width. Also assertscontainedandthumbare left-aligned (figure.x within 100px of card.x).figure.bottom ≤ card.bottom— the invariant the originalfloat: rightdesign violated.handleImageUpload()'s naming scheme, not NULL and not the original.Run locally:
PHPStan: clean on the 5 PHP files touched (pre-existing
storage/plugins/{archives,bibframe-linked-data,discogs}/...warnings onmainremain at 11 and are out of scope here).