Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

fix #287 - checks itemNode is not null in maybeScrollItemIntoView #293

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

Conversation

SkinyMonkey
Copy link

@SkinyMonkey SkinyMonkey commented Nov 2, 2017

When an async function is loading the data:

The itemNode is sometime null in this function : https://github.com/reactjs/react-autocomplete/blob/master/lib/Autocomplete.js#L245.

In the ref function of the renderMenu member function (https://github.com/reactjs/react-autocomplete/blob/master/lib/Autocomplete.js#L437), the ref parameter is null too and sets the refs[item- + index] to null

I tried to not set the key in the ref function if the value was null but it didn't help.

So ultimately I decided to protect the call to scrollIntoView to avoid the exception.

I hope it can fix the problem until you find a better solution if there is one.

PS : sorry, i forgot to read the contribution rules before commiting. My commit is prefixed by fix though.

@eduduardo
Copy link

Same as #281

@SkinyMonkey
Copy link
Author

I'll close it as soon as the other pull request is merged : it's been needed for a long time, a little reminder for the admins/maintainers doesn't hurt.

@eduduardo
Copy link

That's right 😄

@nikmilson
Copy link

nikmilson commented Nov 20, 2017

What about merge this PR? I'm using 1.5.5 version of react-autocomplete with rendering menu to body (due to issues with overflow: hidden blocks) and everything is working great. But from 1.5.7 I get error with nullable itemNode on first render with highlighted item (e. g. arrow down press). I need an update because of renderInput is useful feature for me.

/cc @CMTegner

@reduxdj
Copy link

reduxdj commented Dec 26, 2017

I am still having issues with this always being null, so quickly i monkey-patched this function, by using inheritance ( fast and dirty). checkout the component scroll-into-view, it's pretty sweet.

I also don't show scrollbars in my app because they are king of unnecessary, this stops the outer window form bouncing.

maybeScrollItemIntoView() {
   const stopMainContentFromScrolling = () => {
     const mainContent = window.document.getElementsByClassName('main_content')[0]
     const contentY = mainContent.scrollTop
     if (mainContent.style) {
       mainContent.style.overflowY = 'hidden'
       mainContent.style.top = contentY
       setTimeout(() => {
         mainContent.style['overflow-y'] = 'auto'
         mainContent.style.top = 0
       }, 1000)
     }
   }

   if (this.isOpen() && this.state.highlightedIndex !== null) {
     const { className } = this.props
     const { highlightedIndex } = this.state
     const elements = window.document.getElementsByClassName(className)
     const itemNode = elements[highlightedIndex]
     stopMainContentFromScrolling()
     scrollIntoView(itemNode)
   }
 }

@CMTegner
Copy link
Collaborator

CMTegner commented Feb 4, 2018

As I've mentioned before, this is most likely due to a mismatch between what you are passing in as props.items and the render-tree that your renderMenu method is returning.

This is documented:

Ensure the returned tree includes every entry in items or else the highlight order and keyboard navigation logic will break.

If you think that this is not the case I'd be happy to look over a reduced test case to confirm that there is in fact a different bug in play.

@kaskazurek
Copy link

I got this error (and several others) when I was trying to use a functional component in the render* prop. I did not know that this library uses refs. As you know, functional components do not support refs. I think it should be mentioned in the documentation.

Example (react 15.6.1): https://codesandbox.io/s/32lz0wmmm1
Example (react 16.2.0): https://codesandbox.io/s/8xxq2l5910

With react 16 there is additional warning:

Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants