5
\$\begingroup\$

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'));
\$\endgroup\$
  • 2
    \$\begingroup\$ How do you mean, split them up. You actually didn't split them up, but created methods inside your component. Personally I would choose to inject components that handle how the items in the typeahead should look like and pass that into the search list component, also you could include events that can be used when an item got selected, and I don't really think that handling the current selected item should be the responsibility of the searchlist though that might be a bit much \$\endgroup\$ – Icepickle Aug 8 '17 at 18:32
  • \$\begingroup\$ Thanks for your feedback. So the logic for the typeahead should be moved into the different components? Like the logic for selecting the item should be in the same component which handles the look of the selectedItemList? Or should i keep the general logic in a main file in which i also injects the other components? Hopes it makes sense \$\endgroup\$ – Fripo Aug 8 '17 at 19:11
4
\$\begingroup\$

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 .

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>)
}
\$\endgroup\$
  • \$\begingroup\$ Thanks alot for your review and your time! I see you "complain" about calling setState in handleOnchange too often. Well, i kinda want to update the state, since i want the dropdown to update on every input in order to get an updates list. Is there anyway you see a better solution than what im currently doing? Also the remove() method is defined elsewhere in my code :) \$\endgroup\$ – Fripo Aug 8 '17 at 20:31
  • \$\begingroup\$ If you want to process something before the component is rendered, intercepting changes, you have to look to the component lifecycle. facebook.github.io/react/docs/react-component.html So move the code that need to intercept some updates in the proper methods. \$\endgroup\$ – Mario Santini Aug 8 '17 at 20:54
  • \$\begingroup\$ About setState(), every time you call one cycle is run. Every changes is queued. So when you call it more than a time on a single method is meaningles, as the state you access in the method is still unchanged. Please refer to the documentation: facebook.github.io/react/docs/react-component.html#setstate \$\endgroup\$ – Mario Santini Aug 8 '17 at 20:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.