table_default card_grid_class, MediaGallery hide-Add-at-limit, draggable_list responsive cols - #561
Conversation
…-branch) Lets consumers override card-view grid density (column count, gaps) without touching the per-view-mode `display` utility the component sets itself. Default stays the previous 1/2/3/4-col layout. The attr value must NOT carry a `display` class (`grid`/`hidden`) so controlled table mode keeps emitting only `hidden` — no reliance on Tailwind hidden-beats-grid source order.
When the selection reaches max_count (or 1 in :single mode), the Add tile is omitted entirely rather than rendered as a disabled, greyed-out tile.
`cols` now takes either an integer (1..6, mapped to a static grid-cols-N as before) or a string of Tailwind grid-column classes used verbatim — e.g. "grid-cols-4 lg:grid-cols-6 2xl:grid-cols-8" — so consumers can render a responsive thumbnail grid. MediaGallery plumbs the value straight through; its `cols` doc is updated to match. Backwards compatible: integer callers are unchanged.
timujinne
left a comment
There was a problem hiding this comment.
PR #561 Review
Verdict: PASS with one MEDIUM bug to fix
Stage 1: Spec Compliance — PASS
All three commits implement what was requested and are backwards compatible:
-
table_defaultcard_grid_class— default matches previous hardcoded value;displayutility still set per-branch attable_default.ex:334-336; doc warns against includingdisplayclasses. No existing call-sites broken. -
MediaGallery hide Add tile —
:ifguard correctly combinesnot @readonly+not selection_at_limit?(...)atmedia_gallery.html.heex:75; disabled-state classes anddisabledattr removed; doc updated. -
draggable_listcolsinteger|string — binary guard clause correctly placed before integer clauses atdraggable_list.ex:168; default remains4; doc mentions Tailwind literal scanning requirement. MediaGallery moduledoc updated.
Stage 2: Code Quality
BUG - MEDIUM: Empty grid cell when Add tile is hidden
media_gallery.html.heex:71-85 + draggable_list.ex:156-160
When selection_at_limit? returns true, the :if on the <button> (line 75) prevents the button from rendering, but the <:add_button> slot itself is still declared. In draggable_list.ex:156, the check @add_button != [] evaluates to true because Phoenix slots are lists of slot entries — the list is non-empty regardless of the inner :if. The <div class="sortable-ignore"> wrapper renders as an empty div, creating a visible empty grid cell.
Fix: move the :if guard from <button> to the <:add_button> slot:
<:add_button :if={not @readonly and not selection_at_limit?(@selected, @mode, @max_count)}>
<button type="button" class="..." ...>
...
</button>
</:add_button>This excludes the slot entry entirely, so @add_button == [] and the wrapper div is not emitted.
NITPICK: cols attr type :any is broader than needed
draggable_list.ex:87 — accepts atoms, lists, maps, etc. Only integers and strings are valid. No crash risk (catch-all falls back to grid-cols-4), and doc already describes expected types. No action needed.
NITPICK: Tailwind breakpoint cascade
draggable_list.ex:90 — doc doesn't mention ascending breakpoint order requirement, but the example already shows correct ordering and this is standard Tailwind knowledge. No action needed.
Quality Summary: 0 critical, 1 medium, 0 minor, 2 nitpick
Overall: PASS after fixing the empty grid cell — move :if from <button> to <:add_button> slot in media_gallery.html.heex.
Putting `:if` on the inner <button> left the `<:add_button>` slot non-empty, so draggable_list still rendered an empty `sortable-ignore` wrapper cell at the limit. Guarding the slot declaration itself omits the trailing cell entirely. (Review fix for PR BeamLabEU#561.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Three additive, backwards-compatible changes to core components, driven by the Document Creator image-picker work:
table_defaultcard_grid_classattr — lets consumers override the card-view grid density (column count, gaps) without touching the per-view-modedisplayutility the component sets itself. Default keeps the previous 1/2/3/4-col layout. The value must NOT carry adisplayclass (grid/hidden) so controlled table mode keeps emitting onlyhidden— preserving the source-order-independent fix from the post-MediaGallery: add max_count attr + disable Add button at limit #556 follow-up.MediaGalleryhides the Add tile at the selection limit — when the selection reachesmax_count(or 1 in:singlemode) the Add tile is omitted entirely instead of rendered as a disabled, greyed-out tile.draggable_listcolsaccepts a responsive class string —colsnow takes either an integer (1..6, mapped to a staticgrid-cols-Nas before) or a string of Tailwind grid-column classes used verbatim, e.g."grid-cols-4 lg:grid-cols-6 2xl:grid-cols-8", so consumers can render a responsive thumbnail grid.MediaGalleryplumbs the value straight through.All three preserve existing behavior for current call sites (defaults unchanged; integer
colscallers unaffected).Test plan
<.table_default>/<.draggable_list>/<.MediaGallery>usages render unchangedcard_grid_classoverrides density without breaking controlled table-mode hidingdraggable_listwith a responsivecolsstring renders a responsive grid (verify the larger-breakpoint column count actually wins — class must cascade after smaller breakpoints)