non-optimal function bindings at Nav component

[nik72619c]

Nature of issue?

• Found a bug

Details about the bug:

The functions inside the class components like Nav.jsx are bound inside the render function which should not be done since this is gonna make react use reconciliation and cause a re-render, as it would find a new instance of this function every time. So this must be corrected in order to prevent any optimisation issues.

• Web browser and version: Version 72.0.3626.121 (Official Build) (64-bit)
• Operating System: Windows 10

Steps to reproduce this bug:
Not actually a bug but a performance enhancement
add a console.log inside the render method of nav.jsx and cause a re-render

[siddhant1]

Doesn't look like a huge deal to me but will give some performance boost , I will refactor it

[nik72619c]

I'm working on it

[siddhant1]

I don't think it's that big of a deal , let @catarak have a look and then start

[nik72619c]

cool...but this is necessary because its gonna pose problems when the app grows

[nik72619c]

@catarak

[catarak]

@nik72619c i'm definitely not an expert on react optimizations, so good catch! this seems like a fairly simple fix, so go ahead and pick it up.

[meiamsome]

I wonder if we'd be better off collapsing the content Nav component into a data structure and then rendering it from that, which would mean that the callbacks are created only when that structure was created, something like:

class Nav {
  constructor() {
    this.navLinks = [
      {
        key: 'file',
        title: 'File',
        subMenu: [
          {
            key: 'new',
            title: 'New',
            onClick: () => {
              if (!this.props.unsavedChanges) {
                this.props.newProject();
              } else if (this.props.warnIfUnsavedChanges()) {
                this.props.newProject();
              }
              this.setDropdown('none');
            }
          }
        ]
      },
      // etc.
    ];
  }

  render() {
    return (
      <nav>
        <ul>
          {
            menuItems.map(item => (
              <li key={item.key} className={navDropdownState[item.key]}>
                <button
                  onClick={this.toggleDropdown.bind(this, item.key)}
                  onBlur={this.handleBlur}
                  onFocus={this.clearHideTimeout}
                >
                  <span className="nav__item-header">{item.title}</span>
                  <InlineSVG className="nav__item-header-triangle" src={triangleUrl} />
                </button>
                <ul className="nav__dropdown">
                  <button
                    onClick={this.toggleDropdown.bind(this, item.key)}
                    className="nav__dropdown-heading"
                  >
                    <span>{item.title}</span>
                    <InlineSVG src={triangleUrl} />
                  </button>
                  {
                    item.subMenu.map(subItem => (
                      <li key={subItem.key} className="nav__dropdown-item">
                        <button
                          onClick={subItem.onClick}
                          onFocus={this.handleFocus.bind(this, item.key)}
                          onBlur={this.handleBlur}
                        >
                          {subItem.title}
                        </button>
                      </li>
                    ))
                  }
                </ul>
              </li>
            ))
          }
        </ul>
      </nav>
    );
  }
}

Possibly splitting out those two maps into their own mini-components.

That would be my suggestion, so that the Nav file reduces its current HTML duplication where it possibly can, and you don't get changing callbacks on re-render

[catarak]

@meiamsome i think it would be great to split up the navigation into smaller components and remove duplication!

[siddhant1]

https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

Also I think we should install a webpack plugin to measure performance , personally I don't think this will give a lot of advantage considering the work

[nik72619c]

@catarak will work on this issue !

[nik72619c]

cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

Also I think we should install a webpack plugin to measure performance , personally I don't think this will give a lot of advantage considering the work

I personally don't feel the need for this right now !

[catarak]

@nik72619c i agree with you. i assume there's a bunch of low hanging fruit with respect to code loading time that can be fixed, since it hasn't been a focus for this project.

[nik72619c]

ref #918

[nik72619c]

@catarak kindly see and update me of any changes!