Repository navigation
Add previous/next album controls to the drawer - #306
Conversation
The feed now acts as the drawer's album source, so the player can step through its stories. When stepping past the last loaded story, the page is scrolled to the bottom so the feed pages in more, and the drawer waits for them before moving on. The selected story is scrolled into view. Only the story list is walked: the new-releases carousel above it repeats stories in hidden slides. Also ignore album loads that resolve after the drawer moved on, and toggle the preview button against the album the drawer is actually showing.
|
@IanHoar I’ll take a look at this soon. Probably you saw that on Label/Artist pages this behaviour exists just without the buttons? As in — clicking next on the last track of an album goes to the next album. I like the idea of explicit buttons so if this isn’t setup to work on Label/Artists pages too — let me know and I can look at adding |
|
I actually hadn't seen that yet. I'll double check that I haven't broken anything there, and that this is working on those pages |
|
Using this now to catch up on my feed and I feel like I'm in Bandcamp nirvana |
On the feed, previews were bound by the feed and again by the label view's page-wide pass, so one click opened the drawer and the second handler saw the album already open and minimized it.
The discography grid now opts into the drawer's previous/next album controls and scrolls the selected album into view, like the feed.
IanHoar
changed the title
a115b90 to
42b0a0b
Compare
sabjorn
left a comment
sabjorn
left a comment
There was a problem hiding this comment.
basically ready to go -- just a bit of test coverage missing and a few suggestions
| // Feed previews are bound by the feed and again by the label view's page-wide pass. | ||
| if (button.dataset.besBound === 'true') return; |
There was a problem hiding this comment.
| // Feed previews are bound by the feed and again by the label view's page-wide pass. | |
| if (button.dataset.besBound === 'true') return; | |
| const feedPreviewsBounded = button.dataset.besBound === 'true'; | |
| if (feedPreviewsBounded) return; |
| const seen = new Set<string>(); | ||
| const items: DiscographyItem[] = []; | ||
|
|
||
| // Only walk the story list; the new-releases carousel above it repeats stories in hidden slides. |
There was a problem hiding this comment.
make sure you have a test covering this instead of a comment
| // Only walk the story list; the new-releases carousel above it repeats stories in hidden slides. |
| return items; | ||
| } | ||
|
|
||
| // The feed pages in more stories as the reader nears the bottom, so scroll there and wait for them. |
There was a problem hiding this comment.
| // The feed pages in more stories as the reader nears the bottom, so scroll there and wait for them. |
| }); | ||
| } | ||
|
|
||
| function feedContainer(): HTMLElement { |
There was a problem hiding this comment.
/nit /non-blocking
unless it is important to tests -- I generally prefer inline calls for this type of single line function -- i.e. directly calling in the code rather than adding a layer of abstraction
const feedContainer = document.getElementById('stories') || document.body;
I understand there are risks since maybe this changes one day and it has to be updated in multiple places -- but that is a good time to refactor into a function.
| const discographySource: AlbumSource = { | ||
| extract: extractDiscographyOrder, | ||
| reveal: item => item.element.scrollIntoView?.({ behavior: 'smooth', block: 'center' }), | ||
| showAlbumControls: true | ||
| }; | ||
|
|
||
| let source: AlbumSource = discographySource; |
There was a problem hiding this comment.
why create just to copy over? discographySource isn't used anywhere else, right?
| const discographySource: AlbumSource = { | |
| extract: extractDiscographyOrder, | |
| reveal: item => item.element.scrollIntoView?.({ behavior: 'smooth', block: 'center' }), | |
| showAlbumControls: true | |
| }; | |
| let source: AlbumSource = discographySource; | |
| let source: AlbumSource = { | |
| extract: extractDiscographyOrder, | |
| reveal: item => item.element.scrollIntoView?.({ behavior: 'smooth', block: 'center' }), | |
| showAlbumControls: true | |
| }; |
| if (requestedAlbumId !== albumId) { | ||
| log.debug(`Album ${albumId} arrived after the drawer moved on`); | ||
| return; |
| } | ||
|
|
||
| async function skipAlbum(load: () => Promise<boolean>): Promise<void> { | ||
| if (albumNavLoading) return; |
| await vi.advanceTimersByTimeAsync(1000); | ||
|
|
||
| await expect(loading).resolves.toBe(false); | ||
| vi.useRealTimers(); |
There was a problem hiding this comment.
needs to be in the afterEach or else it can fail to be cleaned up if the line above fails
There was a problem hiding this comment.
test the change the enableFetchCaching default change for loadNextAlbum
it('keeps the caching preference when hopping albums', async () => {
discography.updateDiscographyOrder();
await player.loadAlbumIntoDrawer('123', 'album', true);
await player.loadNextAlbum(); // bare call — exercises the default
expect(vi.mocked(createFetchFunction)).toHaveBeenLastCalledWith(true);
});
Inline the feed container and discography source, swap comments for named values and tests, clean up timers in afterEach, and drop the skipAlbum guard in favour of the disabled buttons it duplicated. Add tests for the stale album guard, double clicks while loading, and the caching preference carrying over when hopping albums.
|
🤩 thanks for forging a path for this one. I find it extremely enjoyable to navigate my feed now |
2 participants
Adds Previous album / Next album controls to the side player on the feed and on label/artist pages. They step through the page's releases and scroll the selected one into view. On the feed, going past the last loaded story loads more.
Screen.Recording.2026-10-01.at.11.32.26.AM.compressed.mp4
Review notes
discography.tsnow takes a pluggable album source; label pages keep the existing grid, and the feed registers its own inpages/feed.ts.Testing