Skip to content

[WC-3505]: Gallery pagination design properties are not applied - #2388

Open
yordan-st wants to merge 12 commits into
mainfrom
fix/WC-3505_gallery-design-properties-not-updated
Open

[WC-3505]: Gallery pagination design properties are not applied#2388
yordan-st wants to merge 12 commits into
mainfrom
fix/WC-3505_gallery-design-properties-not-updated

Conversation

@yordan-st

@yordan-st yordan-st commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Pull request type

Bug fix (non-breaking change which fixes an issue)


Description

Gallery's Pagination design property (Left / Center) has done nothing since the pagination overhaul. Its CSS keyed off a .widget-gallery-pagination wrapper that the overhaul deleted, so pagination was always right-aligned regardless of the setting.

Restoring it is not a CSS change. Pagination now lives in a three-zone flex bar (*-start / *-middle / *-end) where the zone, not the bar, decides position, and those zones are already used by the selection counter and the load more button. Overriding justify-content inside the end zone would centre pagination at ~83% of the bar width, and doing it with CSS order or grid placement would reorder visually while leaving DOM order fixed — a WCAG 2.4.3 (Focus Order) and 1.3.2 (Meaningful Sequence) defect for a paging control.

So placement is decided in markup by a pure function, resolveZones:

alignment pagination zone displaced to end zone untouched
Left *-start selection counter load more stays in *-middle
Center *-middle load more button counter stays in *-start
Right *-end nothing everything

The rule is total: at most three occupants, three zones, and custom pagination replaces the built-in bar rather than adding to it, so at most one occupant is ever displaced. Displacement was chosen over wrapping to a second row because the counter appears dynamically at selected > 0 — wrapping would shift the page the moment a user selects their first item.

The same result drives the footer, the top bar and the editor preview, so they cannot drift apart. That drift is what produced the second and third fixes below.

Also fixed here

  • Custom pagination ignored Position of pagination at runtime — it always rendered below the gallery. Above grid now renders it in the top bar. Both renders it once in the footer, because a widgets placeholder rendered twice would duplicate widget instances, DOM ids and state; check() now surfaces a warning explaining that.
  • The editor preview disagreed with runtime on that same setting, showing custom pagination above the gallery while the app rendered it below.

Design property changes (data-widgets)

  • Pagination is now a toggle button group with Atlas align icons, matching every other alignment control in Studio Pro, and gains an explicit Right option.
  • Property and option names are deliberately unchanged. Studio Pro stores design property selections by property and option name, not by CSS class, so renaming raises CE6083 and CE6087 in every existing app until a developer runs "Update all renamed design properties in project". Verified in Studio Pro during development.
  • Dead .widget-gallery-pagination rules removed; fc-middle / tb-middle are now real flex zones so Center is centred by construction rather than incidentally.

Accessibility note, intended: DOM order stays start → middle → end, so alignment now also determines tab and reading order — Left puts the paging controls before the Clear selection button. Visual and focus order stay in agreement, which the CSS-only alternatives could not achieve.

Not in this PR: DataGrid 2's -padding-top container-query typo and its identical custom-pagination position bug ship separately so Gallery and DataGrid 2 changes stay reviewable apart. [WC-3547 Data Grid 2: Stack top bar in narrow containers](#2389 (comment))

Tests: 145 unit tests pass (was 131). resolveZones is covered by an exhaustive 24-combination invariant pass; the bar components are asserted against real rendered DOM per zone.

Planning artifacts: packages/pluggableWidgets/gallery-web/openspec/changes/fix-gallery-pagination-placement/.

What should be covered while testing?

Build both packages into a test project:

  • packages/pluggableWidgets/gallery-web
  • packages/modules/data-widgets
  1. Alignment × position. Gallery with Pagination = Paging buttons. For each of Left / Center / Right, check Position of pagination = Below grid, Above grid, Both. Pagination should sit hard left, truly centred on the bar, or hard right, in both bars.
  2. Displacement. Enable multi selection with Show selection count = Bottom. Select an item. With Left, pagination takes the left slot and the count moves right; with Center, the count stays left. Repeat with Show selection count = Top against the top bar.
  3. No layout shift. Selecting the first item must not change the footer height or add a row.
  4. All three occupants. Pagination = Load more, Show total count on, selection active. Center should give: count left, paging status centred, Load more right.
  5. Custom pagination. Enable it and check Above grid (renders in the top bar), Below grid (footer), Both (footer only, plus a warning on the widget in Studio Pro). Confirm alignment moves the custom widgets too.
  6. Narrow width. Shrink the gallery container below 500px — the bar should still stack vertically and centre everything, whatever the alignment.
  7. Design mode. The page editor should place everything where the running app does, including custom pagination.
  8. Keyboard. Tab through both bars for each alignment; focus order must follow visual order.
  9. Upgrade safety. An existing page that already had Left or Center selected must show no errors and keep its selection.

@yordan-st
yordan-st force-pushed the fix/WC-3505_gallery-design-properties-not-updated branch from 267ca2a to b97098c Compare August 18, 2026 09:28
@yordan-st
yordan-st marked this pull request as ready for review August 18, 2026 09:28
@yordan-st
yordan-st requested a review from a team as a code owner August 18, 2026 09:28
Comment thread packages/pluggableWidgets/gallery-web/src/helpers/resolveZones.ts Outdated

@iobuhov iobuhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the biggest changes I need is to replace word "zone" with "slot". Slot is more known term in frontend to denote areas where you can put your elements.
Please rewrite spec, comments, variables and other identifiers. Word "zone" is bit too alien.

Comment thread packages/pluggableWidgets/gallery-web/src/components/GalleryTopBarControls.tsx Outdated
Comment thread packages/pluggableWidgets/gallery-web/src/components/GalleryTopBarControls.tsx Outdated
Comment thread packages/pluggableWidgets/gallery-web/src/components/GalleryFooterControls.tsx Outdated
@yordan-st
yordan-st force-pushed the fix/WC-3505_gallery-design-properties-not-updated branch from bd1edd0 to 8060d99 Compare August 21, 2026 13:53
@yordan-st
yordan-st requested a review from iobuhov August 21, 2026 13:53
@github-actions

This comment has been minimized.

gjulivan
gjulivan previously approved these changes Aug 31, 2026
@yordan-st
yordan-st force-pushed the fix/WC-3505_gallery-design-properties-not-updated branch from 8060d99 to 4ea9fb2 Compare September 7, 2026 13:05
@github-actions

This comment has been minimized.

@yordan-st
yordan-st force-pushed the fix/WC-3505_gallery-design-properties-not-updated branch from 595eb63 to 86ccbf4 Compare September 8, 2026 14:55
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/gallery-web/src/helpers/pagingAlignment.ts New: parses pagination alignment from design-property class names
packages/pluggableWidgets/gallery-web/src/helpers/resolveSlots.ts New: pure function mapping alignment + element visibility to bar slots
packages/pluggableWidgets/gallery-web/src/view-models/GalleryRoot.viewModel.ts Added pagingAlignment computed from props.class
packages/pluggableWidgets/gallery-web/src/components/GalleryFooterControls.tsx Rewritten to render from resolveSlots map; fixes custom pagination position
packages/pluggableWidgets/gallery-web/src/components/GalleryTopBarControls.tsx Rewritten; adds tb-middle slot; custom pagination honoured
packages/pluggableWidgets/gallery-web/src/Gallery.editorConfig.ts Adds check() warning for custom pagination + both
packages/pluggableWidgets/gallery-web/src/Gallery.editorPreview.tsx Unified with runtime via resolveSlots; custom pagination preview fixed
packages/pluggableWidgets/gallery-web/src/components/__tests__/GalleryBarControls.spec.tsx New: integration tests for slot placement and custom pagination position
packages/pluggableWidgets/gallery-web/src/helpers/__tests__/pagingAlignment.spec.ts New: unit tests for parsePagingAlignment
packages/pluggableWidgets/gallery-web/src/helpers/__tests__/resolveSlots.spec.ts New: exhaustive invariant tests for resolveSlots
packages/pluggableWidgets/gallery-web/src/view-models/__tests__/GalleryRoot.viewModel.spec.tsx Added tests for pagingAlignment computed
packages/pluggableWidgets/gallery-web/CHANGELOG.md User-facing entries for both fixes
packages/modules/data-widgets/src/themesource/datawidgets/web/design-properties.json Pagination converted to ToggleButtonGroup with icons; adds Right option
packages/modules/data-widgets/src/themesource/datawidgets/web/_gallery-design-properties.scss Removes dead CSS rules; keeps class names as documented contract
packages/modules/data-widgets/src/themesource/datawidgets/web/_gallery.scss Gives fc-middle/tb-middle explicit flex sizing; expands narrow-width container query
packages/modules/data-widgets/CHANGELOG.md User-facing entry for design property rework
packages/pluggableWidgets/gallery-web/openspec/changes/fix-gallery-pagination-placement/ Planning artifacts (design, proposal, specs, tasks)

Skipped (out of scope): dist/, pnpm-lock.yaml, openspec markdown artifacts

CI status: several checks still in progress at review time; Snyk security/snyk reported a failure (unrelated to this change — same pattern seen on other PRs). All completed build and code checks passed ✅


Findings

⚠️ Low — resolveSlots fallback silently overwrites end slot if both counter and loadMore are displaced

File: packages/pluggableWidgets/gallery-web/src/helpers/resolveSlots.ts line 373–381

Note: The invariant tests prove this cannot happen today — custom pagination replaces the built-in bar, so pagination + counter + loadMore are never all three visible at once in the footer. However the function signature accepts hasCounter, hasLoadMore, and hasPagination as independent booleans; nothing in the type system prevents a caller from passing all three as true when alignment is left. In that case counter would be placed in end, then loadMore would also resolve to end (its primary middle is free, so it actually stays in middle — this is fine). Let me restate: center + counter + loadMore puts counter in start and loadMore in end. left + counter + loadMore: pagination takes start, loadMore stays in middle (primary free), counter goes to end. No overwrite actually occurs. The invariant tests confirm this. This is a non-issue — noting it only because the "fallback is always end" pattern could be surprising to a future contributor adding a fourth element.

Consider adding a short comment beside DEFAULT_SLOTS noting that both fallbacks converge on end deliberately, and that this is safe precisely because at most one displacement can occur per bar (custom pagination replaces the built-in bar).


⚠️ Low — FooterPagination and GalleryFooterControls duplicate the showCustomPagination condition

File: packages/pluggableWidgets/gallery-web/src/components/GalleryFooterControls.tsx lines 775 and 792

Note: showCustomPagination is computed identically in both FooterPagination (pgConfig.customPaginationEnabled && pgConfig.pagingPosition !== "top") and the outer GalleryFooterControls. The outer one feeds hasPagination to resolveSlots, while the inner one decides what to render in the slot. They stay in sync by construction today, but a future change to one is easy to miss. Same pattern in GalleryTopBarControls. Not a bug; just a minor DRY opportunity if the components are ever revisited.


⚠️ Low — Multi-line JSDoc on a getter that is already self-explanatory

File: packages/pluggableWidgets/gallery-web/src/view-models/GalleryRoot.viewModel.ts lines 1401–1408

Note: Per repo conventions, comments should only document non-obvious WHY, not WHAT. The five-line JSDoc block on pagingAlignment explains what the getter does (reads from props.class) and how MobX reactivity works — both derivable from the code. The valuable part is the cross-package contract note, which could be a single line referencing pagingAlignment.ts. Consider trimming to just that line to stay consistent with the rest of the class.


Positives

  • Exhaustive invariant test suite — the resolveSlots spec generates all 24 alignment × occupancy combinations and asserts both uniqueness and completeness. This is exactly the right level of coverage for a pure function with combinatorial inputs.
  • Accessibility-first design decision — the choice of TSX placement over CSS order/grid was made explicitly to keep DOM and visual order in agreement (WCAG 2.4.3 and 1.3.2). This is documented in the design artifact and surfaced in the PR description, making the trade-off reviewable.
  • Cross-package class contract documentedpagingAlignment.ts names the three class strings, explains the coupling to design-properties.json, and warns what breaks if they are renamed. This replaces a silent dependency with a written contract.
  • Single resolveSlots call drives footer, top bar and editor preview — the root cause of the original preview/runtime divergence was three separate placement decisions; the fix collapses them to one shared pure function.
  • Backward-compatible design property change — keeping the existing Left/Center option names (and their classes) avoids the CE6083/CE6087 migration burden on consumers. The reasoning is explained in both the PR description and the design doc, which makes it easy to revisit intentionally later.
  • Both + custom pagination handled safely — rendering the placeholder once (footer) and adding an editor check() warning is the right call; duplicating a widgets node would break instance identity and DOM ids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants