Gallery component (scrollable) #327
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Gallery component with enhanced features such as keyboard/scroll navigation, URL hash updates, and improved accessibility announcements.
- Added comprehensive Jest and Playwright tests covering various interactions and initialization modes.
- Provided example configurations (webpack, jest) and updated the README to detail usage, options, API, and events.
Reviewed Changes
Copilot reviewed 29 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gallery/jest.config.js | New Jest configuration for the gallery package. |
| packages/gallery/example/webpack.config.js | Example Webpack configuration to demonstrate gallery usage and hot-reloading. |
| packages/gallery/example/src/js/index.js | Basic example to initialize the gallery inline. |
| packages/gallery/tests/playwright/playwright.spec.js | Playwright UI tests covering keyboard navigation and fullscreen support. |
| packages/gallery/tests/jest/updateURL.js | Jest tests verifying URL update functionality on gallery navigation. |
| packages/gallery/tests/jest/store.js | Tests for the internal state management (store) used by the Gallery component. |
| packages/gallery/tests/jest/interactions.js | Tests covering click interactions and navigation through buttons and direct item selection. |
| packages/gallery/tests/jest/initialisation/multiple.js | Tests ensuring that multiple galleries can be independently initialized and interacted with. |
| packages/gallery/tests/jest/initialisation/manual.js | Tests for manual initialization; highlights that images do not load until initialization. |
| packages/gallery/tests/jest/initialisation/index.js | Tests for default initialization behavior and DOM capturing. |
| packages/gallery/tests/jest/hashchange.js | Tests ensuring that hashchange events correctly update the active gallery item when appropriate. |
| packages/gallery/tests/jest/fromURL.js | Tests for setting the initial active index based on the URL hash. |
| packages/gallery/tests/jest/events.js | Tests verifying that proper events (initialised and change) are dispatched on gallery actions. |
| packages/gallery/tests/jest/api.js | Tests exposing and validating the Gallery API methods. |
| packages/gallery/tests/jest/announcements.js | Tests for live region announcements and custom announcement messages for accessibility. |
| packages/gallery/README.md | Documentation update including installation, usage, options, API, and events of the gallery. |
Files not reviewed (4)
- .azurite/azurite_db_blob.json: Language not supported
- .azurite/azurite_db_blob_extent.json: Language not supported
- packages/gallery/.npmignore: Language not supported
- packages/gallery/package.json: Language not supported
Add live region tests
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mjbp
force-pushed
the
feature/gallery-scroll
branch
from
ef3ce74 to
4cdd2fc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements an alternative Gallery component with keyboard scroll support and improved accessibility features, intended to eventually deprecate the older modal-gallery component. Key changes include test updates (Jest and Playwright), new initialization options, URL updating logic, and enhanced accessible announcements.
Reviewed Changes
Copilot reviewed 29 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gallery/jest.config.js | Adds Jest configuration specific to the gallery package |
| packages/gallery/example/webpack.config.js | Provides an example Webpack config for building the gallery |
| packages/gallery/example/src/js/index.js | Demonstrates basic gallery initialization |
| packages/gallery/tests/* | A suite of tests validating gallery functionality and API |
| packages/gallery/README.md | Updates gallery usage documentation with installation and API info |
Files not reviewed (4)
- .azurite/azurite_db_blob.json: Language not supported
- .azurite/azurite_db_blob_extent.json: Language not supported
- packages/gallery/.npmignore: Language not supported
- packages/gallery/package.json: Language not supported
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Gallery component by introducing a new scrollable implementation that supports keyboard navigation, URL updates, and enhanced accessibility. Key changes include updated configuration and webpack/example setups, comprehensive integration and unit tests for features such as keyboard and click navigation, hashchange URL updates and accessible announcements, and improvements in the component API.
Reviewed Changes
Copilot reviewed 29 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gallery/jest.config.js | New Jest configuration for the Gallery component tests. |
| packages/gallery/example/webpack.config.js | Example webpack configuration to bundle the Gallery component demos. |
| packages/gallery/example/src/js/index.js | Simple instantiation of the Gallery component. |
| packages/gallery/tests/* | Extensive tests for keyboard navigation, URL updates, events, API functions, multiple instance initialisation, manual initialisation, hashchange handling, and accessibility announcements. |
| packages/gallery/README.md | Updated documentation covering usage, configuration options, API methods, and events. |
Files not reviewed (4)
- .azurite/azurite_db_blob.json: Language not supported
- .azurite/azurite_db_blob_extent.json: Language not supported
- packages/gallery/.npmignore: Language not supported
- packages/gallery/package.json: Language not supported
Comments suppressed due to low confidence (1)
packages/gallery/tests/jest/updateURL.js:103
- The button responsible for navigating to the next image uses the class name 'gallery__previous', which can be misleading. Consider renaming it to 'gallery__next' for clarity and consistency with its functionality.
<button class="gallery__previous" aria-label="Next image" data-gallery-next>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new Gallery component with enhanced keyboard scrollability and improved accessibility. The change set includes updates to configuration files, extensive Jest and Playwright tests covering URL handling, event dispatching, API methods, state management, initialization, and accessible announcements, as well as updated documentation in the README.
Reviewed Changes
Copilot reviewed 29 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gallery/jest.config.js | Sets up Jest configuration for the gallery tests with a custom Babel wrapper. |
| packages/gallery/example/webpack.config.js | Provides a webpack configuration for running an example of the gallery component. |
| packages/gallery/example/src/js/index.js | Initializes the gallery instance for the example usage. |
| packages/gallery/tests/playwright/playwright.spec.js | Defines Playwright tests for keyboard navigation, URL updates, scrolling support, and fullscreen behavior. |
| packages/gallery/tests/jest/updateURL.js | Tests the URL update functionality when gallery items change. |
| packages/gallery/tests/jest/store.js | Validates the state management and side effect invocation in the gallery store. |
| packages/gallery/tests/jest/interactions.js | Covers click interactions for next, previous, and specific item navigation. |
| packages/gallery/tests/jest/initialisation/multiple.js | Tests initialization of multiple gallery instances and independent API behaviors. |
| packages/gallery/tests/jest/initialisation/manual.js | Checks manual initialization and corresponding event dispatching. |
| packages/gallery/tests/jest/initialisation/index.js | Verifies initialization warnings, proper DOM capturing, and return values. |
| packages/gallery/tests/jest/hashchange.js | Validates gallery behavior on hash change events, ensuring proper slide selection. |
| packages/gallery/tests/jest/fromURL.js | Ensures the active slide is correctly set based on the URL hash during initialization. |
| packages/gallery/tests/jest/events.js | Checks that the gallery dispatches proper events (initialised and change) on state transitions. |
| packages/gallery/tests/jest/api.js | Tests the availability and functionality of the gallery API methods. |
| packages/gallery/tests/jest/announcements.js | Validates accessible announcements including warnings when the live region is missing and custom announcement rendering. |
| packages/gallery/README.md | Updates documentation detailing usage, options, API, events, tests, and installation. |
Files not reviewed (4)
- .azurite/azurite_db_blob.json: Language not supported
- .azurite/azurite_db_blob_extent.json: Language not supported
- packages/gallery/.npmignore: Language not supported
- packages/gallery/package.json: Language not supported
| broadcast(store, EVENTS.INITIALISED)(state); | ||
| }; | ||
|
|
||
| const writeLiveRegion = ({ activeIndex, items, settings, dom }) => dom.liveRegion.innerHTML = sanitise(settings.announcement(activeIndex + 1, items.length)); |
There was a problem hiding this comment.
Would it be taking things too far to have this conditionally done based on a setting? The reason I ask is I was experimenting with the CSS, and found that things still worked really quite nicely - even when changing the grid-auto-columns to 50%, or 33% - for more slider like behaviour. The only thing that didn't work was tracking the active card.
At that point, if more than one is visible - an active slide doesn't really make sense, but removing the live region from the markup only errored. Might be making things more complicated than is needed though!
|
|
||
| </ul> | ||
|
|
||
| <button class="gallery__next" aria-label="Next image" data-gallery-next> |
There was a problem hiding this comment.
When I was trying out examples on iOS Voiceover, it felt a bit better having the Next/Previous buttons as siblings in the markup. When they were either side, it felt like I was "accidentally" scrolling the gallery and getting to the end of it, when I was just trying to reach the 'Next' button.
This is just my opinion though of course!! And comes with all the usual caveats around me not being an AT power user and just going by what feels right to me personally!
Also, I'm very aware that this is just the example. The example for this component feels quite important though, compared to the others - since a lot of the functionality is tied up in really specific markup and css.
| }); | ||
| }); | ||
|
|
||
| test.describe('Gallery > Full screen mode support', { tag: '@all' }, () => { |
There was a problem hiding this comment.
I think this test set is something different now! It should maybe be 'Gallery > URL Update' or something like that
|
|
||
| test.describe('Gallery > Full screen mode support', { tag: '@all' }, () => { | ||
| test('Should update the URL when item changes', async ({ page }) => { | ||
| const list = await page.locator('[data-gallery-list]'); |
There was a problem hiding this comment.
Just plain page.locator calls to cache variables don't need the await, it doesn't harm anything though! Just Visual Studio always politely asks me to remove them
| () => { | ||
| if (!options.fromScroll) list.scrollLeft = (next * items[next].clientWidth); | ||
| }, | ||
| writeLiveRegion, |
There was a problem hiding this comment.
Have we maybe lost the line that dealt with focus here in the other branch?
I noticed when trying the buttons in NVDA, because focus stays on the next/prev buttons in this version, we're only getting '2 of 4' read out, rather than the details of the new slide we got in the previous one.
There was a problem hiding this comment.
Second this one! Currently only slide number read out, which doesn't seem quite right.
| scroll-snap-type: x mandatory; | ||
| overflow: scroll hidden; | ||
| scroll-behavior: auto; | ||
| overscroll-behavior: contain; |
There was a problem hiding this comment.
When focus is active inside a non-fullscreen gallery list area ([like this example](https://test-gallery-scroll-2025.netlify.app/)), swiping to scroll back up or down the page doesn't seem to work. It feels a bit like you're stuck, unless you figure out you can scroll from outside of the list section, which might not be immediately obvious to users.
It seems to be an effect of the overscroll-behavior: contain property. Is this in place to prevent users accidentally scrolling up/down while swiping through horizontal slides? I think that makes sense to want to prevent, but just worth being aware it has a slight trade-off in that regard!
| () => { | ||
| if (!options.fromScroll) list.scrollLeft = (next * items[next].clientWidth); | ||
| }, | ||
| writeLiveRegion, |
There was a problem hiding this comment.
Second this one! Currently only slide number read out, which doesn't seem quite right.
| if (scrollPosition === 0 && itemWidth === 0) return; | ||
| if (Number.isNaN(itemWidth) || Number.isNaN(scrollPosition)) return; | ||
| if (scrollPosition === 0) return 0; | ||
| return Math.round(scrollPosition / itemWidth); |
There was a problem hiding this comment.
When the last gallery item is active, I can tab/click on the next button to cycle back to the start of the gallery but I'm unable to swipe right on a laptop touchpad to do this. Same behaviour when trying to navigate from first gallery item to the last by swiping left.
Are there some checks we can add here to cycle the position properly when scrolling/swiping? "Overscroll" doesn't seem to be an event we can listen for. I'll try and experiment with this in my own time, but thought worth bringing up.
Closes #193
This is an alternative (better?) implementation of the component described in #193, and PR created #323.
This PR adds a new Gallery component that can be used inline or in a modal, with a few improvements over the existing modal-gallery component, with a view to deprecating it.
The difference between PR #323 is that this gallery is keyboard scrollable, similar(ish) to the modal gallery used on the guardian website - https://www.theguardian.com/football/2025/mar/19/newcastle-united-fans-carabao-cup-final-photo-essay.
Supporting scroll completely changes how changing active item works internally (so making scroll support a config option is not really viable), and it relies on newer CSS scroll and scroll snapping rule support. I think it also makes it more accessible, but would like that challenged.