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

onStepEnter isn't being called in gatsby #40

Closed
jjrchrds opened this issue May 24, 2020 · 20 comments
Closed

onStepEnter isn't being called in gatsby #40

jjrchrds opened this issue May 24, 2020 · 20 comments

Comments

@jjrchrds
Copy link

jjrchrds commented May 24, 2020

I'm trying to get the example working in Gatsby. The component renders, but onStepEnter is never actually called:

import React, { Component } from "react"
import { Scrollama, Step } from 'react-scrollama';

import Layout from "../components/layout"
import Head from '../components/head';

class NarrativePage extends Component {
  state = {
    data: 0,
    steps: [10, 20, 30],
    progress: 0,
  };

  onStepEnter = ({ element, data }) => {
    console.log(data);
    element.style.backgroundColor = 'lightgoldenrodyellow';
    this.setState({ data });
  };

  onStepExit = ({ element }) => {
    element.style.backgroundColor = '#fff';
  };

  onStepProgress = ({ element, progress }) => {
    this.setState({ progress });
  };

  render() {
    const { data, steps, progress } = this.state;
    
    return (
      <Layout>
        <Head title="Narrative"/>
        <div className="graphicContainer">
          <div className="scroller">
            <Scrollama
              onStepEnter={this.onStepEnter}
              onStepExit={this.onStepExit}
              progress
              onStepProgress={this.onStepProgress}
              offset={0.4}
              debug
            >
              {steps.map(value => (
                <Step data={value} key={value}>
                  <div className="step">
                    <p>step value: {value}</p>
                    {value === data && <p>{Math.round(progress * 100)}%</p>}
                  </div>
                </Step>
              ))}
            </Scrollama>
          </div>
          <div className="graphic">
            <p>{data}</p>
          </div>
        </div>
      </Layout>
    )
  }
}

export default NarrativePage;
@jsonkao
Copy link
Owner

jsonkao commented May 26, 2020

We just published v2.2.8, which waits for window load (rather than component mount) to calculate offset heights and intersection margins (4d6a52b). Could you try updating your React Scrollama and seeing if the bug persists?

@maerzhase
Copy link
Contributor

maerzhase commented Jul 7, 2020

Hi there! first of all thanks for the lib. I really like the simplicity of the api. I ran into an issue that is related to the fix you are mentioning for this issue.

Since you are now "calculate(ing) offset heights and intersection margins" only on document.load and not on component mount <Scrollama /> is only usable in true SPA's. When you mount <Scrollama /> after the document was loaded you have no way of using the lib anymore. This is easily the case when e.g. you have a next.js application and navigate from one page to another.

Could you elaborate why this change was introduced? To me it feels counter intuitive to trigger the calculation only on docoument.load. I believe you actually should calculate anything on componentDidMount rather explicitly triggering the whole lib on document.load.

@olegrjumin
Copy link

Ran into same issue @maerzhase is describing, tried loading via dynamic import in next.js with ssr:false option, on document.load it works, and then on route change stops working

@jsonkao
Copy link
Owner

jsonkao commented Jul 12, 2020

Ah, thanks so much for the explanations @maerzhase @olegrjumin. Apologies, I've never really worked with non SPAs before. Would a solution be to move the calculations back to componentDidMount but also add a calculation trigger on document.load for redundancy? The reason why I had initially moved the calculations to on-page-load in the first place was that the library needs to access DOM element offsets and heights, but sometimes those properties aren't accurate/available yet at componentDidMount. I'm not sure if this is a universal React thing, or there's some sort of race condition between different component renders that create this inaccuracy.

@jjrchrds
Copy link
Author

jjrchrds commented Jul 12, 2020

no luck for me still.

this works

`
componentDidMount() {
const scrollama = require("scrollama")
const scrollThreshold = 0.33
this.scroller = scrollama()

this.scroller
  .setup({
    container: "#narrative-scroll",
    step: ".narrative-step",
    threshold: scrollThreshold,
    progress: true,
    debug: false,
  })
  .onStepEnter(this.handleScrollStepEnter)
  .onStepExit(this.handleScrollStepExit)
  .onStepProgress(this.handleProgress)

// setup resize event
window.addEventListener("resize", this.scroller.resize)

}`

@maerzhase
Copy link
Contributor

From the react documentation doing the whole calculation in componentDidMountshould be the exact place to do so:

https://reactjs.org/docs/react-component.html#componentdidmount

The DOM is supposed to be available. Could you share an example were you ran into the problem that you could not retrieve DOM elements on mount? Maybe this was in a early version of react?

Same goes for the original issue here. @jjrchrds could you share an runnable environment that shows your issue? From what I undestand going from componentDidMount to document.load never actually solved the original issue...

@goleary
Copy link

goleary commented Jul 15, 2020

@maerzhase thanks for your fix, It unblocked me!

@goleary
Copy link

goleary commented Jul 16, 2020

@maerzhase I may have spoken too soon. My website runs with gatsby develop, but will not build (gatsby build). Are you able to build your site using your forked repo?

Looks like the lib is trying to reference window at build time:
"window" is not available during server side rendering.

@maerzhase
Copy link
Contributor

Jeah this will always be the case you have to opt out server side rendering for this lib, since there is no window on the server. Usually you do this with dynamic imports. e.g. https://nextjs.org/docs/advanced-features/dynamic-import

@goleary
Copy link

goleary commented Jul 17, 2020

@maerzhase thanks for the suggestion. I tried out an implementation using loadable-components:

import loadable from '@loadable/component'

const Scrollama = loadable(() => import('~utilities/react-scrollama'), {
  resolveComponent: components => components.Scrollama,
})

const Step = loadable(() => import('~utilities/react-scrollama'), {
  resolveComponent: components => components.Step,
})

But now I'm getting a new error when trying to execute the built bundle:

Could not get step with id react-scrollama-0                       react-dom.production.min.js:209

I guess getting this working on Gatsby is going to be quite a bit more work than I originally anticipated.

EDIT:

Turns out I just needed to move my code around taking into account how Gatsby works (get underlying data at build time and pass the data to a loadable component at runtime). Doh!

@maerzhase
Copy link
Contributor

yeah figured that this has nothing to do with the library per-se because i am using it in production builds + ssr. the lib could address ssr though (or at least mention it in the readme)

@maxkrieger
Copy link

Simple reproduction of the issue, by copy pasting the code in the readme https://codesandbox.io/s/fervent-brown-v0l20?file=/src/App.js

@maerzhase
Copy link
Contributor

Simple reproduction of the issue, by copy pasting the code in the readme https://codesandbox.io/s/fervent-brown-v0l20?file=/src/App.js

clearly still the same issue. Initialising the lib on document.load is just not sufficient for many use-cases.

@jsonkao
Copy link
Owner

jsonkao commented Aug 13, 2020

Apologies for the delay. I'm willing to use componentDidMount. There is still this issue however. What if we called the "domDidLoad" function both in CDM and document.load?

@maerzhase
Copy link
Contributor

but sometimes those properties aren't accurate/available yet at componentDidMount.

Could you provide an example for this case? Usually DOM related properties should be available on componentDidMount otherwise React would have a huge problem in the first place, no?

@jsonkao
Copy link
Owner

jsonkao commented Aug 17, 2020

I will create an example repo some time today/tomorrow. For now, I will push a version with redundant code. If I can't reproduce it tomorrow, I will remove the redundancy.

@jsonkao
Copy link
Owner

jsonkao commented Aug 17, 2020

This is released in v2.2.11 @jjrchrds @maerzhase @maxkrieger @goleary @olegrjumin

@maerzhase
Copy link
Contributor

maerzhase commented Aug 18, 2020

The codesandbox from @maxkrieger still doesn't work with v2.2.11

https://codesandbox.io/s/fervent-brown-v0l20?file=/src/App.js

@jsonkao you totally ignored what i wrote in #47. If you use progress in gatsby and/or next.js you will end up with an error because this.state.offsetHeight is undefined. Why you didn't look at the PR? if you just wanted to introduce a quick-fix this was the solution for it. Your change is not a sufficient solution sorry.

The error you will see is:
TypeError: Failed to construct 'IntersectionObserver': The provided double value is non-finite.
because of undefined variables in rootMargin option provided to the IntersectionObserver

@jsonkao
Copy link
Owner

jsonkao commented Aug 18, 2020

Ah my bad, I hadn't noticed your changes to Step.js. I'll merge that in now. Thanks for pointing it out again.

@jsonkao
Copy link
Owner

jsonkao commented Aug 18, 2020

Released in v2.2.12 @maerzhase

@jsonkao jsonkao closed this as completed Mar 28, 2021
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

No branches or pull requests

6 participants