Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ensure media info is dispatched after view change #1587

Merged
merged 1 commit into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions src/utils/embla/carousel-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ export class CarouselController {

public selectSlide(index: number): void {
this._carousel.scrollTo(index, this._transitionEffect === 'none');

// This event exists to allow the caller to know the difference between
// programatically force slide selections and user-driven slide selections
// (e.g. carousel drags). See the note in auto-media-loaded-info.ts on how
// this is used.
const newSlide = this.getSlide(index);
if (newSlide) {
dispatchFrigateCardEvent<CarouselSelected>(this._parent, 'carousel:force-select', {
index: index,
element: newSlide,
});
}
}

protected _refreshCarouselContents = (): void => {
Expand Down Expand Up @@ -162,17 +174,6 @@ export class CarouselController {
};

carousel.on('select', () => selectSlide());
carousel.on('settle', () => {
const carouselSelected = getCarouselSelectedObject();
if (carouselSelected) {
dispatchFrigateCardEvent<CarouselSelected>(
this._parent,
'carousel:settle',
carouselSelected,
);
}
});

return carousel;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,30 @@ declare module 'embla-carousel/components/Plugins' {
}
}

/**
* On the relationship between carousel:select and carousel:force-select:
*
* There is a complex interplay here. `carousel:force-select` is an event
* dispatched by the carousel when it is forced to select a particular slide
* (i.e. the view has changed). `carousel:select` is dispatched for any
* selection -- forced or human (e.g. the user dragging the carousel).
*
* The media info should only be dispatched _after_ the view object has been
* updated (since the view will clear the loaded media info). The setting of the
* view (trigged by `carousel:select`) may require async fetches and may take a
* while -- and so if the card dispatched media on `carousel:selecte` then the
* media info may be dispatched before the view is set (which could result in
* the dispatched media immediately being cleared by the view).
*
* It is fine to have media info dispatched from the `carousel:init` event,
* since the carousel will be initialized based on a particular view object. In
* practice, the carousel will be initialized before the media is loaded, so
* there may not be anything to dispatch at that point.
*
* When media is loaded, that media loaded info will always be allowed to
* propogate upwards as long as it is selected.
*/

type AutoMediaLoadedInfoType = CreatePluginType<LoosePluginType, LooseOptionsType>;

function AutoMediaLoadedInfo(): AutoMediaLoadedInfoType {
Expand All @@ -30,7 +54,9 @@ function AutoMediaLoadedInfo(): AutoMediaLoadedInfoType {
}

emblaApi.on('init', slideSelectHandler);
emblaApi.on('select', slideSelectHandler);
emblaApi
.containerNode()
.addEventListener('frigate-card:carousel:force-select', slideSelectHandler);
}

function destroy(): void {
Expand All @@ -40,7 +66,9 @@ function AutoMediaLoadedInfo(): AutoMediaLoadedInfoType {
}

emblaApi.off('init', slideSelectHandler);
emblaApi.off('select', slideSelectHandler);
emblaApi
.containerNode()
.removeEventListener('frigate-card:carousel:force-select', slideSelectHandler);
}

function mediaLoadedInfoHandler(ev: CustomEvent<MediaLoadedInfo>): void {
Expand Down
25 changes: 18 additions & 7 deletions tests/utils/embla/carousel-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,24 +110,35 @@ describe('CarouselController', () => {
it('should select given slide', () => {
const children = createTestSlideNodes();
const parent = createParent({ children: children });

const forceSelectListener = vi.fn();
parent.addEventListener('frigate-card:carousel:force-select', forceSelectListener);

const carousel = new CarouselController(createRoot(), parent);

carousel.selectSlide(4);

expect(getEmblaApi()?.scrollTo).toBeCalledWith(4, false);
expect(forceSelectListener).toBeCalledWith(
expect.objectContaining({
detail: { index: 4, element: children[4] },
}),
);
});

it('should dispatch settle event', () => {
const children = createTestSlideNodes();
it('should not select non-existent slide', () => {
const children = createTestSlideNodes({ n: 10 });
const parent = createParent({ children: children });
new CarouselController(createRoot(), parent);

const settleHandler = vi.fn();
parent.addEventListener('frigate-card:carousel:settle', settleHandler);
const forceSelectListener = vi.fn();
parent.addEventListener('frigate-card:carousel:force-select', forceSelectListener);

callEmblaHandler(getEmblaApi(), 'settle');
const carousel = new CarouselController(createRoot(), parent);

carousel.selectSlide(11);

expect(settleHandler).toBeCalled();
expect(getEmblaApi()?.scrollTo).toBeCalledWith(11, false);
expect(forceSelectListener).not.toBeCalled();
});

it('should dispatch select event', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
} from '../../../../../src/utils/media-info';
import { createMediaLoadedInfo, createParent } from '../../../../test-utils';
import {
callEmblaHandler,
createEmblaApiInstance,
createTestEmblaOptionHandler,
createTestSlideNodes,
Expand All @@ -30,7 +29,6 @@ describe('AutoMediaLoadedInfo', () => {
plugin.destroy();

expect(emblaApi.off).toBeCalledWith('init', expect.anything());
expect(emblaApi.off).toBeCalledWith('select', expect.anything());
});

describe('should correctly propogate media load/unload depending on whether media is currently selected', () => {
Expand Down Expand Up @@ -82,11 +80,15 @@ describe('AutoMediaLoadedInfo', () => {
dispatchExistingMediaLoadedInfoAsEvent(children[5], createMediaLoadedInfo());

vi.mocked(emblaApi.selectedScrollSnap).mockReturnValue(4);
callEmblaHandler(emblaApi, 'select');
emblaApi
.containerNode()
.dispatchEvent(new Event('frigate-card:carousel:force-select'));
expect(mediaLoadedHandler).not.toBeCalled();

vi.mocked(emblaApi.selectedScrollSnap).mockReturnValue(5);
callEmblaHandler(emblaApi, 'select');
emblaApi
.containerNode()
.dispatchEvent(new Event('frigate-card:carousel:force-select'));
expect(mediaLoadedHandler).toBeCalled();
});
});
Loading