Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3761: NavBar made only once #3779

Merged
merged 1 commit into from Feb 21, 2018

Conversation

Projects
None yet
4 participants
@Akarshit
Copy link
Contributor

commented Feb 16, 2018

Resolves #3761
Impact: minor
Type: bugfix|performance

Issue

The issue was that the getComponent call was making a new component everytime. So the whole component tree from NavBar was being made again.

Solution

The components are now made in the constructor and are reused in the render() this cause the internal checks of React in deciding when to render something again work properly.

Testing

  1. In a mobile browser while on the homepage clicking the shop title/logo should do nothing.

To really test

  1. Before merging this PR
  2. Add a contructor() or componentWillUnmount() here
  3. Add a breakpoint inside the constructor/function
  4. Click on the brand title on the homepage
  5. The code will reach the breakpoint, which mean the component is made again.
  6. Merge this PR
  7. Again click on the brand title on the homepage
  8. The control will not reach the breakpoint, which means the component was not made again.

@Akarshit Akarshit requested review from spencern and jshimko and removed request for spencern Feb 16, 2018

return (
<div className={pageClassName} id="reactionAppContainer">
render() {
const { actionViewIsOpen, structure } = this.props;

This comment has been minimized.

Copy link
@nnnnat

nnnnat Feb 16, 2018

Member

I feel like you could have the code from lines 28 - 46 live inside a lifecycle method (componentWillMount, componentWillUpdate) and only have the JSX return inside the render method.

This comment has been minimized.

Copy link
@Akarshit

Akarshit Feb 20, 2018

Author Contributor

I thought that might be out of scope for this bug, but yes maybe it makes sense to move them now.
Thanks :)

This comment has been minimized.

Copy link
@aaronjudd

aaronjudd Feb 21, 2018

Contributor

@jshimko approved this so we're merging it, if that wasn't expected and @Akarshit @nnnnat if this is worth doing please add the updates as a new PR.

@aaronjudd aaronjudd merged commit c7de6e0 into release-1.8.0 Feb 21, 2018

4 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No new issues
Details

@aaronjudd aaronjudd deleted the fix-3761-akarshit-navbar branch Feb 21, 2018

@spencern spencern referenced this pull request Feb 22, 2018

Merged

Release 1.8.0 #3645

@spencern spencern referenced this pull request Mar 9, 2018

Merged

Release 1.9.0 #3941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.