I believe you need to add a componentShouldUpdate lifecycle method to update the state when new props are passed into this component.

Right now the component is not updating based on incoming props. Reproduction steps:

• Make sure you're logged out
• Go to /
• Refresh and it should have logo, links, and login button
• Click "login" button
• The navbar still has logo, links, and login button
• Refresh
• The navbar is updated to just have logo

Can't repro anymore after f66d3b0 - let me know if it works when you get the chance!

Yeah I can't reproduce it anymore. But wouldn't it be better to have displayType and buttonType in the state? The only reason why this component re-renders properly is because its container is re-rendering itself. If for some reason the container does not re-render itself, but the props change, then this component wouldn't re-render.

Hm, I’ll have a closer look at this tonight

@mingyokim sorry, I'm not sure I understand - doesn't a prop change force the container to re-render? From what I understand, displayType and buttonType are only manipulated from here, and the navbar itself does not change them:

https://github.com/nwhacks/nwhacks2019/blob/master/web/containers/navbar/index.js#L57

It also seems like setting props that a component doesn't manipulate into state is somewhat bad practice (https://stackoverflow.com/a/47341539), but there also seems to be some other APIs for doing this kind of stuff (https://stackoverflow.com/a/49839457, https://stackoverflow.com/a/32414771)... though to be honest this seems to be kind of murky territory, since from what I understand, this type of stuff should just be kept in props (https://stackoverflow.com/a/40330195). I could be missing something though...

Officially, react only distinguishes between class and function (potentially stateful and stateless respectively), and:

The above two components are equivalent from React’s point of view.

https://reactjs.org/docs/components-and-props.html#functional-and-class-components

so props do cause a render on stateful components? oops i was mistaken, it's all good then

Similar question: https://www.reddit.com/r/reactjs/comments/7tn79g/state_and_componentwillreceiveprops/

class Foo extends Component {
  state = { value: this.props.value }

  // If the props change, update state
  componentWillReceiveProps (nextProps) {
    if (nextProps.value !== this.props.value) {
      this.setState({ value: nextProps.value })
    }  
  } 

  render () {
    // render based on state.value
  }
}

Top response:

Why you don't render props directly?

An update can be caused by changes to props or state.

https://reactjs.org/docs/react-component.html

Okay, so clearly change in props cause a re-render, reactjs says so. My bad lol

If this is the case, definitely use props rather than state because the component is not modifying the buttonType or displayType, they're just displaying it.

Approving

I'll just do one last sanity check

curious, why is this needed?

This keeps the navbar above all other elements, so long as we avoid using z-index elsewhere

spaces should always be multiples of 4 - can you change this to 4px? If the design said 3px that should be an error in design. @sherryyx can you confirm?

This is a hack because the logo looked slightly off :(

4px looks fine too though - I'll make this change

Yeah I can't reproduce it anymore. But wouldn't it be better to have displayType and buttonType in the state? The only reason why this component re-renders properly is because its container is re-rendering itself. If for some reason the container does not re-render itself, but the props change, then this component wouldn't re-render.

Can we store 96 into some kind of variable?

Sure - 96 is in the CSS as navbar height, but I’ll replicate it here

same here

cleaann but can we name this something like linksAndButton? it doesn't just have links

True!

nice