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

[v2] add listener to ScrollCaptor, even when no scroll is available on mount #2861

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

pyreta
Copy link
Contributor

@pyreta pyreta commented Jul 27, 2018

Current Behavior

The onMenuScrollToBottom callback will only fire if the dropdown menu is scrolling on mount.

Proposed Changes

The onMenuScrollToBottom callback always fires, regardless of whether or not the dropdown menu was scrollable on mount.

Why Change?

A dropdown menu may render more options after mounting and therefore event listeners may need to be listening for scroll events.

Use case

In a project I'm working on, I'm creating a dropdown supporting pagination that loads asynchronously. When I mount my Select component, my initial state is empty. Once the user starts typing, an async request is made loading my state with options, and showing them in the dropdown. However since the dropdown mounted with no options to start with, the ScrollCaptor is not listening for scroll events. This prevents me from being able to load the next page on bottom scroll after loading options asynchronously.

CodeSandbox Example

You can see an example here where opening the dropdown with options already loaded allows for the onMenuScrollToBottom callback to log "bottom" in the console. However if you clear the options before opening the dropdown, the callback will not fire after options have loaded.

@jrhalchak
Copy link

👍 Got a vote from me. I'm currently forcing the height to be smaller than I know most results will be.

@vincentjames501
Copy link

+1

3 similar comments
@Brunomm
Copy link

Brunomm commented Aug 8, 2018

+1

@chuvikovd
Copy link

+1

@dpetranek
Copy link

+1

@jossmac
Copy link
Collaborator

jossmac commented Nov 23, 2018

@pyreta this is a good idea, though I worry about binding a listener for every menu; especially when many won't have need for it.

What about using a ResizeObserver to watch for changes?

@jossmac
Copy link
Collaborator

jossmac commented Nov 23, 2018

For those adding +1 comments please just hit 👍on the original post. We get it, you want this feature, please don't add unnecessary noise.

piotr-jagielski pushed a commit to piotr-jagielski/react-select that referenced this pull request Dec 14, 2018
@Kemosabert
Copy link

Kemosabert commented Sep 10, 2019

Hey @jossmac, is there anything I can help with to get this issue resolved? Would investigating the ResizeObserver option help here?

@jossmac
Copy link
Collaborator

jossmac commented Sep 11, 2019

@pyreta @Kemosabert if you can resolve the conflicts I'll merge it. DW about the rest.

@jossmac jossmac self-requested a review September 11, 2019 02:10
@Kemosabert
Copy link

@jossmac Thanks! I created a new PR for this: #3752 because I don't know if @pyreta saw this message. When the PR is merged into master, how long approximately before this fix is available in a version on NPM?

@pyreta pyreta force-pushed the fix-onMenuScrollToBottom-bug branch from cb1c574 to 7c61e0f Compare September 11, 2019 13:14
@pyreta
Copy link
Contributor Author

pyreta commented Sep 11, 2019

@jossmac @Kemosabert conflicts have been resolved. Ready to merge!

@pyreta pyreta force-pushed the fix-onMenuScrollToBottom-bug branch from 7c61e0f to 05a7fe3 Compare September 11, 2019 15:09
@jossmac jossmac merged commit 691a011 into JedWatson:master Sep 12, 2019
@jossmac
Copy link
Collaborator

jossmac commented Sep 12, 2019

how long approximately before this fix is available in a version on NPM?

Not sure, sorry mate. I don't have publishing permission.

@Dalitzky
Copy link

Is there anyone else experiencing issues with the scroll not working after this merge ?
For me this merge caused an issue when using react-select on top of modal window with menuPortaltarget && menuShouldBlockScroll.

@romanfazulianov
Copy link

Hi everyone!
I faced vertical scroll lock after update to version 3.0.5 and higher. If I create fully dynamic menu list component, that will be recreated on every render, scroll works. However, there is another issue instead - option hover scrolls menu list to top.

rollback to 3.0.4 fixes any problems with scroll, but causes warnings about old lifecycle methods

Any thoughts?

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.