Carousel: Set controlled index to newly added cards

[Mitch-At-Work]

Previous Behavior

Controlled index implementation, user adds a CarouselCard and sets index to index of newly added card (simultaneous state update).
As index is not registered at time of state change, state is not updated.

New Behavior

Controlled index implementation, user adds a CarouselCard and sets index to index of newly added card (simultaneous state update).
Engine will update to new card in this scenario only.

[fabricteam]

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
react-components react-components: entire library	1.161 MB 290.837 kB	1.162 MB 290.865 kB	199 B 28 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.372 kB 35.755 kB

_{🤖 This report was generated against 0ee7c4ca1e888545ae516c231f48f4881a738809}

[layershifter]

@Mitch-At-Work do you have a reproduction case for this issue? I tried to build a simple repro:

import {
  Body1,
  Button,
  Divider,
  makeStyles,
  mergeClasses,
  Title1,
  tokens,
  Tooltip,
  Toolbar,
  ToolbarButton,
} from '@fluentui/react-components';
import {
  Carousel,
  CarouselAnnouncerFunction,
  CarouselButton,
  CarouselCard,
  CarouselSlider,
} from '@fluentui/react-components';
import * as React from 'react';

const useClasses = makeStyles({
  carousel: {
    display: 'grid',
    gridTemplateColumns: '1fr auto 1fr',
    gridTemplateRows: '1fr auto',
    gap: '10px',
    placeItems: 'center',
    // width: '300px',
  },
  container: {
    display: 'flex',
    flexDirection: 'column',
    gap: '20px',
  },
  carouselSlider: {
    display: 'flex',
    overflow: 'hidden',
  },

  footer: {
    display: 'flex',
    gap: '10px',

    alignSelf: 'center',
    width: 'max-content',

    border: `${tokens.strokeWidthThin} solid ${tokens.colorNeutralStroke1}`,
    borderRadius: tokens.borderRadiusMedium,
    boxShadow: tokens.shadow16,

    padding: '10px',
  },
  controls: {
    padding: 0,
  },
  controlButton: {
    minWidth: '32px',
  },
  code: {
    display: 'flex',
    placeItems: 'center',
    padding: '4px 8px',

    fontSize: tokens.fontSizeBase200,
    lineHeight: tokens.lineHeightBase200,

    backgroundColor: tokens.colorNeutralBackground4,
    borderRadius: tokens.borderRadiusMedium,
  },

  card: {
    // display: 'block',
    // width: '500px',
    // maxWidth: '500px',
    // flex: 'initial',
  },
  wireframe: {
    backgroundColor: tokens.colorNeutralBackground3,
    border: `${tokens.strokeWidthThin} solid ${tokens.colorNeutralStroke1}`,

    display: 'flex',
    flexDirection: 'column',
    gap: '12px',
    placeContent: 'center',

    padding: '40px',
    height: '200px',

    position: 'relative',
  },
  wireframeEven: {
    backgroundColor: tokens.colorBrandBackground2,
    border: `${tokens.strokeWidthThin} solid ${tokens.colorBrandStroke1}`,
  },
  wireframeInfo: {
    position: 'absolute',
    right: '12px',
    top: '12px',

    backgroundColor: tokens.colorPaletteRedBackground2,
    border: `${tokens.strokeWidthThin} dotted ${tokens.colorPaletteRedBorder2}`,

    fontSize: tokens.fontSizeBase200,
    padding: '4px 8px',
  },
});

const getAnnouncement: CarouselAnnouncerFunction = (index: number, totalSlides: number, slideGroupList: number[][]) => {
  return `Carousel slide ${index + 1} of ${totalSlides}`;
};

const WireframeContent: React.FC<{
  index: number;
}> = props => {
  const classes = useClasses();

  return (
    <div className={mergeClasses(classes.wireframe, props.index % 2 === 0 && classes.wireframeEven)}>
      <div className={classes.wireframeInfo}>
        <code>index: {props.index}</code>
      </div>
      <Title1 align="center">Lorem Ipsum</Title1>
      <Body1 align="center">Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor...</Body1>
    </div>
  );
};

export const Controlled = () => {
  const [activeIndex, setActiveIndex] = React.useState(1);

  const classes = useClasses();
  const [allCardsVisible, setAllCardsVisible] = React.useState(false);

  return (
    <div className={classes.container}>
      <Carousel
        activeIndex={activeIndex}
        className={classes.carousel}
        groupSize={1}
        onActiveIndexChange={(e, data) => setActiveIndex(data.index)}
        announcement={getAnnouncement}
      >
        <Tooltip content="Go To Previous Page" relationship="label">
          <CarouselButton navType="prev" aria-label="Previous Carousel Page Button" />
        </Tooltip>

        <div className={classes.carouselSlider}>
          <CarouselSlider>
            <CarouselCard aria-label="1 of 5" className={classes.card}>
              <WireframeContent index={0} />
            </CarouselCard>
            <CarouselCard aria-label="2 of 5" className={classes.card}>
              <WireframeContent index={1} />
            </CarouselCard>
            <CarouselCard aria-label="3 of 5" className={classes.card}>
              <WireframeContent index={2} />
            </CarouselCard>
            <CarouselCard aria-label="4 of 5" className={classes.card}>
              <WireframeContent index={3} />
            </CarouselCard>
            <CarouselCard aria-label="5 of 5" className={classes.card}>
              <WireframeContent index={4} />
            </CarouselCard>

            {allCardsVisible && (
              <CarouselCard aria-label="5 of 7" className={classes.card}>
                <WireframeContent index={5} />
              </CarouselCard>
            )}
            {allCardsVisible && (
              <CarouselCard aria-label="5 of 7" className={classes.card}>
                <WireframeContent index={6} />
              </CarouselCard>
            )}
          </CarouselSlider>
        </div>

        <Tooltip content="Go To Next Page" relationship="label">
          <CarouselButton navType="next" aria-label="Next Carousel Page Button" />
        </Tooltip>
      </Carousel>

      <div className={classes.footer}>
        <code className={classes.code}>{JSON.stringify({ allCardsVisible, activeIndex }, null, 2)}</code>
        <Divider vertical />
        <Toolbar className={classes.controls}>
          {new Array(allCardsVisible ? 7 : 5).fill(null).map((_, index) => (
            <ToolbarButton
              key={`toolbar-button-${index}`}
              aria-label={`Carousel Nav Button ${index} `}
              className={classes.controlButton}
              appearance="subtle"
              disabled={index === activeIndex}
              onClick={() => setActiveIndex(index)}
            >
              {index}
            </ToolbarButton>
          ))}
        </Toolbar>
        <Button
          appearance="primary"
          onClick={() => {
            setAllCardsVisible(true);
            setActiveIndex(6);
          }}
        >
          Add cards
        </Button>
      </div>
    </div>
  );
};

And it works on master & the latest release (https://stackblitz.com/edit/n2khkq?file=src%2Fexample.tsx) 🤔

[Mitch-At-Work]

@Mitch-At-Work do you have a reproduction case for this issue? I tried to build a simple repro:

And it works on master & the latest release (https://stackblitz.com/edit/n2khkq?file=src%2Fexample.tsx) 🤔

Have a test story I was using here: https://github.com/Mitch-At-Work/fluentui/blob/user/mifraser/story-test-new-cards/packages/react-components/react-carousel/stories/src/Carousel/CarouselControlled.stories.tsx

I'll double check this is working on master, previously it was not but perhaps latest Embla upgrade is able to handle.

Edit: Can confirm this works on master if an animation occurs, i.e. if you are on the 1st index and add a new card it will work.

It will not work however if 'jump' is set to true, or if the users is on the last card already, i.e. 5 cards total and user is on 5th card when adding a 6th

[layershifter]

Edit: Can confirm this works on master if an animation occurs, i.e. if you are on the 1st index and add a new card it will work.

It will not work however if 'jump' is set to true, or if the users is on the last card already, i.e. 5 cards total and user is on 5th card when adding a 6th

Good catch 👍 Indeed it reproes.

https://github.com/davidjerleke/embla-carousel/blob/67a1ac0d8821a2eadf6232e9663fef59a21a9abd/packages/embla-carousel/src/components/SlidesHandler.ts#L31-L32

As I see, Embla also does reinit and dispatches a matching event on DOM mutation.

What if we will instead move out handleReinit() to the hook body (useEventCallback() will allow us to read the latest value from render without hacks) and call always scrollTo() there? I.e.:

-  const handleReinit = () => {
+  const handleReinit = useEventCallback(() => {
    const nodes: HTMLElement[] = emblaApi.current?.slideNodes() ?? [];
    const groupIndexList: number[][] = emblaApi.current?.internalEngine().slideRegistry ?? [];
    const navItemsCount = groupIndexList.length > 0 ? groupIndexList.length : nodes.length;

    const data: CarouselUpdateData = {
      navItemsCount,
      activeIndex: emblaApi.current?.selectedScrollSnap() ?? 0,
      groupIndexList,
      slideNodes: nodes,
    };

    for (const listener of listeners.current) {
      listener(data);
    }

+   emblaApi.current?.scrollTo(activeIndex);
-  };
+  });

  const viewportRef: React.RefObject<HTMLDivElement> = React.useRef(null);
  const containerRef: React.RefObject<HTMLDivElement> = React.useMemo(() => {

[Mitch-At-Work]

Todo before merge: Add E2E cypress tests

EDIT: Will land E2E tests separately