Various Issues w/ Imported Slides

[ebrillhart]

We've been seeing a variety of bugs that all have one thing in common - slides that are being imported from elsewhere in the project and are not co-located with the <Deck /> component. Related issues (that I've closed in favor of focusing on the overarching problem) include:

• Appear tags used in imported slides not working, even though they work as documented in non-imported Slides Appear tag doesn't work out of the box #666

• Various issues with SlideSet and related when not colocated with <Deck /> component

I think we should dig into why any issues would be popping up when a Slide is imported instead of co-located. I'd imagine it has something to do with how <Manager /> handles things.

[bkinsey808]

Thank you. I'm also having this issue. I really don't want co-located slides. But I have to if I want Appear.

[narinluangrath]

The problem is on line 747 of Manager.js

    const targetProps = {
      dispatch: this.props.dispatch, // We're gonna need this prop
      // a bunch of other props too (omitted)
    };

    return (
      <SlideWrapper key={slideIndex} {...slide.props} {...targetProps}>
        {cloneElement(slide, { ...slide.props, ...targetProps })}
      </SlideWrapper>
    );

Before we render the current <Slide />, we attach a bunch of props to it, including Redux's dispatch function. That function gets used in cDM of the Slide component.

  componentDidMount() {
    const slide = this.slideRef;
    const frags = slide.querySelectorAll('.fragment');
    let currentOrder = 0;
    if (frags && frags.length && !this.context.overview) {
      Array.prototype.slice
        .call(frags, 0)
        .sort(
          (lhs, rhs) =>
            parseInt(lhs.dataset.order, 10) - parseInt(rhs.dataset.order, 10)
        )
        .forEach(frag => {
          frag.dataset.fid = currentOrder;
          if (this.props.dispatch) { // This prop came from <Manager />
            this.props.dispatch(
              addFragment({
                className: frag.className || '',
                slide: this.props.hash,
                id: `${this.props.hash}-${currentOrder}`,
                animations: Array.from({ length: frag.dataset.animCount }).fill(
                  this.props.lastSlideIndex > this.props.slideIndex
                )
              })
            );
          }
          currentOrder += 1;
        });
    }

A "fragment" in the code above roughly corresponds to an <Appear /> component. This code essentially stores all the different fragments into Redux. Manager uses this fragment state to determine whether to go to the next slide or to render the next <Appear />.

We start running into problems when we "wrap" <Slide /> with another component (<Test /> in the example below)

export default class Presentation extends React.Component {
  render() {
    return (
      <Deck transition={['zoom', 'slide']} transitionDuration={500} theme={theme}>
        <Test />
        <Slide transition={['zoom']} bgColor="primary">
          <Heading size={1} fit caps lineHeight={1} textColor="secondary">
            Second slide
          </Heading>
        </Slide>
      </Deck>
    )
  }
}

function Test() {
  return (
    <Slide transition={['zoom']} bgColor="primary">
      <Heading size={1} fit caps lineHeight={1} textColor="secondary">
        First slide
      </Heading>
      <Appear>
        <Text textColor="tertiary" size={3}>
          Hello there!
        </Text>
      </Appear>
    </Slide>
  )
}

This code will fail because when Manager does its cloneElement magic (as described above) the props get sent to <Test /> instead of <Slide />. So we never get the dispatch function, and the Slide's fragments never get saved in the store. You can easily fix this by prop drilling, i.e.

function Test(props) { // This will work
  return (
    <Slide transition={['zoom']} bgColor="primary" {...props}>
      <Heading size={1} fit caps lineHeight={1} textColor="secondary">
        First slide
      </Heading>
      <Appear>
        <Text textColor="tertiary" size={3}>
          Hello there!
        </Text>
      </Appear>
    </Slide>
  )
}

I think the real crux of the issue is that Manager is expecting users to only use one of the following as a child: <Slide />, <Magic />, <SlideSet />. If you use something else, there's a good chance the code wont work. If you want to extract code to a separate file, I'd recommend not using a wrapper component and just saving the JSX to a variable.

const Test = (
  <Slide transition={['zoom']} bgColor="primary">
    <Heading size={1} fit caps lineHeight={1} textColor="secondary">
      Test slide
    </Heading>
    <Appear>
      <Text textColor="tertiary" size={3}>
        Hello there
      </Text>
    </Appear>
  </Slide>  
)

export default Test

[maxcan]

One work around is to have you modules export a react fragment and keep your slides top level under the deck:

const Fragment: React.SFC = () => {
  return (
    <>
      <Heading size={4}>Appear Test</Heading>
      <Heading size={2}>line 1</Heading>
      <Appear>
        <Heading size={2}>line 2</Heading>
      </Appear>
      <Appear>
        <Heading size={2}>line 3</Heading>
      </Appear>
    </>
  );
};
const Deck: React.SFC = () => {
  return (
    <Deck>
      <Slide><Fragment/></Slide>
    </Deck>
  );
};

[kale-stew]

Closing as a now-outdated known issue. We should resolve this in the rewrite. Please check out our pinned issue for more info

[forrest-akin]

It appears that this also impacts Stepper. Wrapping Slide contents in a React.Fragment works at least to get the Deck to acknowledge the Slide, but Stepper ignores its steps and continues immediately to the next Slide instead. Any chance there's a known work around for this as well, or a previous version I could use in the mean time instead while Slide is being rewritten?

Edit:
I think the rewrite might have already been released - shall I open a new issue?