React typeahead component

Question

In order to get a better grasp of React I decided to create a typeahead component. I'd really love some feedback on this in order to improve, learn or avoid mistakes regarding my code. I'm unsure whether I should split up my components even more than I already did.

import React from 'react';
import ReactDOM from 'react-dom';
import SearchListItem from './search_list_item.jsx';

class SearchList extends React.Component {

  constructor(props) {
    super(props);

    this.state = {
      cursor: 0,
      data: ['Bananas', 'Apples', 'Ape', 'Oranges', 'Ora', 'Cherries', 'Pears', 'Kiwi'],
      searchedList: [],
      searchInput: '',
      showDropdown: true
    }

    this.handleOnchange = this.handleOnchange.bind(this);
    this.handleKeyPress = this.handleKeyPress.bind(this);
    this.handleClickedItem = this.handleClickedItem.bind(this);
  }

  handleOnchange(value) {
    this.setState({ showDropdown: true })
    this.setState({ searchInput: value });
    const dataItem = this.state.data
    let searchedListItems = dataItem.filter(dataItem => dataItem.includes(this.state.searchInput));
    var listToRender = [];
    if (searchedListItems) {
      listToRender.push(searchedListItems);
    } else {
      var itemToRemove = this.state.searchedListItems
      listToRender.remove(listToRender, itemToRemove);
    }
    this.setState({ searchedList: listToRender });
  }

  handleKeyPress(e) {
    const { cursor, searchedList } = this.state;
    const arrowUp = 38;
    const arrowDown = 40;
    const enter = 13;

    if (e.keyCode === arrowUp && cursor > 0) {
      this.setState( prevState => ({
        cursor: prevState.cursor - 1
      }))
    } else if (e.keyCode === arrowDown && cursor < searchedList[0].length - 1) {
      this.setState( prevState => ({
        cursor: prevState.cursor + 1
      }))
    } else if (e.keyCode === enter && cursor >= 0) {
      this.setState({
        searchInput: searchedList[0][this.state.cursor],
        showDropdown: false,
        cursor: 0
       });
    }
  }

  handleClickedItem(data) {
    this.setState({
      searchInput: data,
      showDropdown: false,
      cursor: 0
    })
  }

  submitForm(e) {
    e.preventDefault();
    alert
  }

  renderSearchList() {
    const { cursor } = this.state
    if (this.state.searchedList[0]) {
      const searchList = this.state.searchedList[0].map((data, index) => {
        return (
          <SearchListItem
            key={index}
            id={index}
            handleClickedItem={this.handleClickedItem.bind(null, data)}
            isActive={cursor === index ? 'active dropdown-li' : 'dropdown-li'}
            data={data} />
        );
      });
      return (
        <div className="dropdown-div">
          <ul className="dropdown-ul">
            {searchList}
          </ul>
        </div>
      );
    }
  }

  render() {
    return (
      <div>
      <form onSubmit={this.submitForm} >
        <input
          className="input"
          type="text"
          value={this.state.searchInput}
          onChange={event => this.handleOnchange(event.target.value)}
          onKeyDown={this.handleKeyPress} />
        </form>
        { this.state.showDropdown ? this.renderSearchList() : <div /> }
      </div>
    );
  }
}

ReactDOM.render(<SearchList/>, document.getElementById('main'));

Answer 1

I think there are many things you could improve, I will try to review your code and give some pointers.

Instead of using React.Component, you should directly access Component:

import React, { Component } from 'react';

class SearchList extends Component {

In older examples, you could find it in the way you do it, but more recent example use this form.

Not sure why you call so many times the setState() method, it is not needed and not a good practice, as every time you call it React will re-render the component.

If you change some data that effects the visualization, you will find out that your component is rendered many times. Of course if you do not change something that impacts the visualization, you won't see, but the engine will run silently, until one day in the far future a small change will turn in such case.

handleOnchange(value) {
  const dataItem = this.state.data
  let searchedListItems = dataItem.filter(dataItem => dataItem.includes(this.state.searchInput));
  var listToRender = [];
  if (searchedListItems) {
    listToRender.push(searchedListItems);
  } else {
    var itemToRemove = this.state.searchedListItems
    listToRender.remove(listToRender, itemToRemove);
  }
  this.setState({ 
      showDropdown: true,
      searchInput: value,
      searchedList: listToRender
  });
}

About variable names: data is a very bad name for a variable, you should avoid. Believe me, I know as I used so many times and eventually I figured out I shouldn't do.

Maybe you can use allItems instead.

About the method logic.

Array.filter() will always return an array even empty. So your check:

if (searchedListItems) {

is always true, you should:

if (searchedListItems.length > 0) {

About: listToRender.remove(listToRender, itemToRemove)

listToRender should be an empty list, no way there are values inside, according to your code. Anyway I'm sure you never arrive here, because javascript Array does not have the method remove().

I think you can just improve your filter, if you want to remove more items.

this.setState({ 
    showDropdown: true,
    searchInput: value,
    searchedList: this.state.allItems
      .filter(dataItem => 
          (dataItem.includes(this.state.searchInput) || 
           this.state.searchedListItems.indexOf(dataItem) !== -1))
});

Or you could assign it directly, as you did't change the list before this line, but if you are going to add more logic the concat method should do the stuff. Then just access the array directly.

I don't know where you saw this thing but don't do it:

this.setState( prevState => ({
  cursor: prevState.cursor - 1
}))

You could just simply do:

this.setState({ cursor: this.state.cursor -1 })

Much better.

Another issue with the following method is that you duplicate to much code and use to many time the this.setState().

handleKeyPress(e) {
  const { cursor, searchedList } = this.state;
  const arrowUp = 38;
  const arrowDown = 40;
  const enter = 13;

  if (e.keyCode === arrowUp && cursor > 0) {
    this.setState( prevState => ({
      cursor: prevState.cursor - 1
    }))
  } else if (e.keyCode === arrowDown && cursor < searchedList[0].length - 1) {
    this.setState( prevState => ({
      cursor: prevState.cursor + 1
    }))
  } else if (e.keyCode === enter && cursor >= 0) {
    this.setState({
      searchInput: searchedList[0][this.state.cursor],
      showDropdown: false,
      cursor: 0
     });
  }
}

You don't have to do it:

handleKeyPress(e) {
  const { 
      searchedList,
      arrowUp,          // you can move this in your state
      arrowDown,        // or better as module const
      enter             // but not inside the method.
  } = this.state;

  let cursorIncr = 1;
  let newState = {
      cursor: this.state.cursor
  };

  if (e.keyCode === arrowUp && cursor > 0) {
    newState.cursor -= 1;
  } else if (e.keyCode === arrowDown && cursor < searchedList[0].length - 1) {
    newState.cursor += 1;
  } else if (e.keyCode === enter && cursor >= 0) {
    newState.cursor = 0;
    newState["searchInput"] = searchedList[0][this.state.cursor];
    newState["showDropdown"] = false;
  }

  this.setState(newState);
}

And now I jump back a bit for this:

listToRender.push(searchedListItems);

I see you read the data like:

searchedList[0]...

Looks like you never care about more lists inside this array of arrays, and it's ok as you always have one. So consider to use concat:

listToRender = listToRender.concat(searchedListItems);

Consider to move this method as a stateless component:

renderSearchList() {
  const { cursor } = this.state
  if (this.state.searchedList[0]) {
    const searchList = this.state.searchedList[0].map((data, index) =>  {          return (
          <SearchListItem
            key={index}
            id={index}
            handleClickedItem={this.handleClickedItem.bind(null, data)}
            isActive={cursor === index ? 'active dropdown-li' : 'dropdown-li'}
            data={data} />
        );
      });
    return (
      <div className="dropdown-div">
        <ul className="dropdown-ul">
          {searchList}
        </ul>
      </div>
    );
  }
}

Like this:

const SearchListItems = ({handleClick, cursor, items}) => {
    const searchItems = items.map((item, index) => (<SearchListItem 
                key={index} 
                id={index} 
                handleClickedItem={handleClick} 
                isActive={cursor == index ? 'active dropdown-li' : 'dropdown-li'} 
                data={item}/>));

    return (<div>{searchItems}</div>)
}

Just a function and it reads the props from the input parameter, using the spread operator of ecmascript-6.

You could have this stateless component in the same file, or place in another, as it is convenient for your use case.

Maybe you could have a better names.

Then change your render to use the new component:

render() {
   return (<div>
    <form onSubmit={this.submitForm} >
      <input
        className="input"
        type="text"
        value={this.state.searchInput}
        onChange={event => this.handleOnchange(event.target.value)}
        onKeyDown={this.handleKeyPress} />
    </form>
    { this.state.showDropdown && 
           <SearchListItems 
            handleClick={this.handleClickedItem.bind(null, data)}
            cursor={this.state.cursor}
            items={this.state.searchedList[0]}/>}
     </div>)
}