0

I searched, I really did and couldn't find the exact answer to what I'm looking for.

I have a component for my nav, it works nicely except when it's in a responsive state, it won't activate the state to hide it again.

What I have is this which works

    class Nav extends React.Component {
  constructor(props) {
    super(props)
    this.addActiveClass= this.addActiveClass;
    this.state = {
      hideNavItems: true,
      active: false,
    };
  }

  toggleMenu() {
    const currentState = this.state.active;
    this.setState({
      hideNavItems: !this.state.hideNavItems,
      active: !currentState

    })
  }

  render() {
    return (
      <Wrapper>
        <Desktop>
          <Navlist className={this.props.navClass}>
            <Navitem className="logo"><a href="welcome" className="menu">Jabba's Crypt</a></Navitem>
            <MenuItem tagline="home" />
            <MenuItem tagline="projects" />
            <MenuItem tagline="about" />
            <MenuItem tagline="blog" />
            <MenuItem tagline="contact" />
          </Navlist>
        </Desktop>
        <Tablet>
          <NavitemLogo onClick={this.toggleMenu.bind(this)} className='logo'><Menu href="#" className="menu"><Sparanwrap><Icon name="bars" /></Sparanwrap>Jabba's Crypt</Menu></NavitemLogo>
          <Navlist hide={this.state.hideNavItems} className={this.state.active ? 'slidein' : 'slideout'}>
            <MenuItem tagline="home" />
            <MenuItem tagline="projects" />
            <MenuItem tagline="about" />
            <MenuItem tagline="blog" />
            <MenuItem tagline="contact" />
          </Navlist>
        </Tablet>
      </Wrapper>
    );
  }
}

export default Nav;

However when I add, say

<MenuItem tagline="projects" onClick={this.toggleMenu.bind(this)} className={this.state.active ? 'slidein' : 'slideout'} />

It doesn't hide the menu again. I know my code is incorrect and I know I'm missing something.

Andrew Li
  • 55,805
  • 14
  • 125
  • 143
sthig
  • 459
  • 3
  • 12
  • 36

2 Answers2

1

setState is asynchronous and can be batched together with other calls, so accessing state inside the call doesn't guarantee previous state. Instead try using a callback and the first argument with previous state to ensure the correct previous state is used:

this.setState(prevState => ({
  hideNavItems: !prevState.hideNavItems,
  active: !prevState.active
}));

Read more about setState in the React documentation.


A few tips to make your code a bit cleaner:

  • Get rid of this.addActiveClass = this.addActiveClass, it does nothing
  • Bind the method inside the constructor so you don't need to do bind(this) in every onClick. It's also more efficient as you don't create a new function every click, this.toggleMenu = this.toggleMenu.bind(this);
Andrew Li
  • 55,805
  • 14
  • 125
  • 143
  • 2
    @Pirs I left a comment on why I felt it was warranted. You can improve your post and I'll happily remove it, there's no need to be rude about it. It's not personal. – Andrew Li May 29 '17 at 19:07
  • @AndrewLi thank you for your help, I'm cleaning a few things up right now with your instructions. as far as the "prevState" - I'm unsure where that goes exactly, it doesn't replace what I have in `toggleMenu()` does it? Thank you for your help, much appreciated! – sthig May 29 '17 at 19:24
  • @sthig It does the same exact thing but it ensures that the state you're using is correct. `setState` doesn't ensure the state you access is correct previous state so using a callback ensures it's correct. – Andrew Li May 29 '17 at 19:27
  • ah, got that part. Everything is working as it was now with the cleaning of the code. However still when I click one of the menus, the class is not changed from slidein to slideout and so it doesn't animate to a hidden state. – sthig May 29 '17 at 19:50
  • am I missing something? – sthig May 29 '17 at 19:51
1

Have you an error ?

Try this code :

 class Nav extends React.Component {
  constructor(props) {
    super(props)
    this.addActiveClass= this.addActiveClass;
    this.state = {
      hideNavItems: true,
      active: false,
    };
    // change this
    this.toggleMenu= this.toggleMenu.bind(this);

  }

  toggleMenu() {
   // and this, from Andrew my love
   this.setState(prevState => ({
     hideNavItems: !prevState.hideNavItems,
     active: !prevState.active
   }));
  }

  render() {
    return (
      <Wrapper>
        <Desktop>
          <Navlist className={this.props.navClass}>
            <Navitem className="logo"><a href="welcome" className="menu">Jabba's Crypt</a></Navitem>
            <MenuItem tagline="home" />
            <MenuItem tagline="projects" />
            <MenuItem tagline="about" />
            <MenuItem tagline="blog" />
            <MenuItem tagline="contact" />
          </Navlist>
        </Desktop>
        <Tablet>
          {// and this}
          <NavitemLogo onClick={this.toggleMenu} className='logo'><Menu href="#" className="menu"><Sparanwrap><Icon name="bars" /></Sparanwrap>Jabba's Crypt</Menu></NavitemLogo>
          <Navlist hide={this.state.hideNavItems} className={this.state.active ? 'slidein' : 'slideout'}>
            <MenuItem tagline="home" />
            <MenuItem tagline="projects" />
            <MenuItem tagline="about" />
            <MenuItem tagline="blog" />
            <MenuItem tagline="contact" />
          </Navlist>
        </Tablet>
      </Wrapper>
    );
  }
}

export default Nav;
pirs
  • 2,410
  • 2
  • 18
  • 25
  • i did it raw love – pirs May 29 '17 at 19:07
  • hi guys, so I tried both your ways and when I click on the menu it doesn't hide (or slide back up). Am I doing something wrong? – sthig May 29 '17 at 19:36
  • Maybe, check the className, is the style do the animation when you're changing it directly in the browser console ? – pirs May 29 '17 at 19:41
  • it does, if I change it from slidein to slideout, it works fine in the console however when I click one of the dropdown menus, the class is not changed – sthig May 29 '17 at 19:46
  • wait!! I got it (I think) – sthig May 29 '17 at 19:52
  • that worked thank you guys both (sorry to see the arguing there for a second). much appreciated!!! – sthig May 29 '17 at 19:52