Skip to content
This repository was archived by the owner on Sep 13, 2020. It is now read-only.

Conversation

@ndbroadbent
Copy link

@ndbroadbent ndbroadbent commented Apr 22, 2017

Instead of checking for this.isOpen !== props.isOpen, we need to check for this.props.isOpen !== props.isOpen, so that the parent component can close the menu force child components to re-render by updating the isOpen prop.

(this.isOpen and this.props.isOpen can get out of sync.)

@ndbroadbent ndbroadbent force-pushed the fix_rerender_on_change branch from 588d70f to 66287b9 Compare April 22, 2017 22:42
@Kureev
Copy link
Owner

Kureev commented Apr 23, 2017

Hi @ndbroadbent. this.props.isOpen & this.isOpen can be out of sync, indeed. This happens because we need to mutate isOpen property every time when we toggle side menu. As you may know, it's a bad practice to mutate props, so we keep a local, props-driven value in the instance property.

Furthermore, we have componentWillReceiveProps that keeps an eye on this.props.isOpen, so I don't see a reason why we need to change this. Can you elaborate?

@Kureev
Copy link
Owner

Kureev commented Jul 9, 2017

Closed due to lack of feedback. Feel free to re-open this PR answering the question above.

@Kureev Kureev closed this Jul 9, 2017
@AlexCatch
Copy link

It would make more sense to have a single source of truth. We either keep the open state entirely in the side menu or we keep it entirely in the parent component, it's not generally recommended to copy props like it is.

It also requires you to call forceUpdate as it's set as a property of the side menu and not inside state, is there a reason why it isn't stored in the component state?

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.

3 participants