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: unregisterScrollListener error in multiple instances of Windowscroller #1584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lsky-walt
Copy link

@lsky-walt lsky-walt commented Aug 26, 2020

Thanks for contributing to react-virtualized!

Before submitting a pull request, please complete the following checklist:

  • The existing test suites (npm test) all pass
  • For any new features or bug fixes, both positive and negative test cases have been added
  • For any new features, documentation has been added
  • For any documentation changes, the text has been proofread and is clear to both experienced users and beginners.
  • Format your code with prettier (yarn run prettier).
  • Run the Flow typechecks (yarn run typecheck).

Here is a short checklist of additional things to keep in mind before submitting:

  • Please make sure your pull request description makes it very clear what you're trying to accomplish. If it's a bug fix, please also provide a failing test case (if possible). In either case, please add additional unit test coverage for your changes. :)
  • Be sure you have notifications setup so that you'll see my code review responses. (I may ask you to make some adjustments before merging.)

When multiple WindowScrollers are bound to the same Dom, unregisterScrollListener error occurs.

*Example: *

<WindowScroller
  ref={this._setRef}
  scrollElement={isScrollingCustomElement ? customElement : window}>
  {({height, isScrolling, registerChild, onChildScroll, scrollTop}) => (
    <div className={styles.WindowScrollerWrapper}>
      <AutoSizer disableHeight>
        {({width}) => (
          <div ref={registerChild}>
            <List
              ref={el => {
                window.listEl = el;
              }}
              autoHeight
              className={styles.List}
              height={height}
              isScrolling={isScrolling}
              onScroll={onChildScroll}
              overscanRowCount={2}
              rowCount={list.size}
              rowHeight={30}
              rowRenderer={this._rowRenderer}
              scrollToIndex={scrollToIndex}
              scrollTop={scrollTop}
              width={width}
            />
          </div>
        )}
      </AutoSizer>
    </div>
  )}
</WindowScroller>

<h1 style={{textAlign: 'center', height: 300, backgroundColor: 'silver', color: 'white', lineHeight: '300px'}}>Second WindowScroller</h1>

<WindowScroller
  ref={this._setRef}
  scrollElement={isScrollingCustomElement ? customElement : window}>
  {({height, isScrolling, registerChild, onChildScroll, scrollTop}) => (
    <div className={styles.WindowScrollerWrapper}>
      <AutoSizer disableHeight>
        {({width}) => (
          <div ref={registerChild}>
            <List
              ref={el => {
                window.listEl = el;
              }}
              autoHeight
              className={styles.List}
              height={height}
              isScrolling={isScrolling}
              onScroll={onChildScroll}
              overscanRowCount={2}
              rowCount={list.size}
              rowHeight={30}
              rowRenderer={this._rowRenderer}
              scrollTop={scrollTop}
              width={width}
            />
          </div>
        )}
      </AutoSizer>
    </div>
  )}
</WindowScroller>
@Aegz
Copy link

Aegz commented Jun 8, 2021

Hey @lsky-walt , do you want to jump in on my PR that does the same thing but has unit tests?

#1660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants