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

Carousel: Drag event based index callback #33092

Conversation

Mitch-At-Work
Copy link
Contributor

Previous Behavior

Carousel was calling the index change callback on Button and Focus events only, preventing users from receiving callback information about drag events.

New Behavior

Drag event now correctly emits a native DOM event with type 'drag', this is driven directly by Embla engine and hence is native.

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 21, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: entire library
1.159 MB
290.329 kB
1.16 MB
290.616 kB
1.126 kB
287 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
69.21 kB
20.174 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
220.616 kB
63.902 kB
react-components
react-components: FluentProvider & webLightTheme
44.447 kB
14.59 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-timepicker-compat
TimePicker
107.387 kB
35.758 kB
🤖 This report was generated against 8fd463bdea974f2402a05a5b621a63ec128a60cf

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 21, 2024

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme virtual-rerender-with-unmount 85 89 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 655 641 5000
Button mount 308 303 5000
Field mount 1115 1162 5000
FluentProvider mount 710 710 5000
FluentProviderWithTheme mount 87 95 10
FluentProviderWithTheme virtual-rerender 39 38 10
FluentProviderWithTheme virtual-rerender-with-unmount 85 89 10 Possible regression
MakeStyles mount 870 843 50000
Persona mount 1775 1764 5000
SpinButton mount 1432 1345 5000
SwatchPicker mount 1657 1652 5000

@Mitch-At-Work Mitch-At-Work marked this pull request as ready for review October 21, 2024 20:14
@Mitch-At-Work Mitch-At-Work requested review from a team as code owners October 21, 2024 20:14
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

The problem that we have there is kinda simple: Embla does not provide access to events.

IMO, it would nicer to solve it via a custom plugin:

export type PointerEventPluginOptions = {
  onSelectViaDrag: (event: PointerEvent | MouseEvent, index: number) => void;
};

export type PointerEventPlugin = CreatePluginType<{}, PointerEventPluginOptions>;

function PointerEventPlugin(options: PointerEventPluginOptions): PointerEventPlugin {
  let emblaApi: EmblaCarouselType;
  let pointerEvent: PointerEvent | MouseEvent | undefined;

  function documentDownListener(event: PointerEvent | MouseEvent) {
    const targetDocument = emblaApi.containerNode().ownerDocument;

    pointerEvent = event;

    targetDocument.removeEventListener('mousedown', documentDownListener);
    targetDocument.removeEventListener('pointerdown', documentDownListener);
  }

  function pointerUpListener() {
    const targetDocument = emblaApi.containerNode().ownerDocument;

    targetDocument.addEventListener('mousedown', documentDownListener);
    targetDocument.addEventListener('pointerdown', documentDownListener);
  }

  function selectListener() {
    if (pointerEvent) {
      const newIndex = emblaApi.selectedScrollSnap() ?? 0;

      options.onSelectViaDrag(pointerEvent, newIndex);
      pointerEvent = undefined;
    }
  }

  function init(emblaApiInstance: EmblaCarouselType, optionsHandler: OptionsHandlerType): void {
    emblaApi = emblaApiInstance;

    emblaApi.on('pointerUp', pointerUpListener);
    emblaApi.on('select', selectListener);
  }

  function destroy(): void {
    emblaApi.off('pointerUp', pointerUpListener);
    emblaApi.off('select', selectListener);
  }

  return {
    name: 'pointerEvent',
    options,
    init,
    destroy,
  };
}

@Mitch-At-Work Mitch-At-Work force-pushed the user/mifraser/carousel-drag-index-change branch from aecd0ce to fbbb055 Compare October 25, 2024 20:02
@Mitch-At-Work Mitch-At-Work force-pushed the user/mifraser/carousel-drag-index-change branch from a51ba44 to 41d6c84 Compare October 28, 2024 17:29
@Mitch-At-Work Mitch-At-Work enabled auto-merge (squash) October 28, 2024 17:31
@Mitch-At-Work Mitch-At-Work merged commit a126ed1 into microsoft:master Oct 28, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants