Repository: react-materialize/react-materialize
Pull request #965 (2019-11-27 22:51:11)

Refactor/navbar hooks
# Description

Part of #928 
Also closes #875 

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

# How Has This Been Tested?

Updated `spec`s and `snapshot`s

# Checklist:

- [x] I have commented my code, particularly in hard-to-understand areas
- [x] My changes generate no new warnings
- [x] I have not generated a new package version. (the maintainers will handle that)



Patch:
diff --git a/src/Navbar.js b/src/Navbar.js
index eab2329..189056e 100644
--- a/src/Navbar.js
+++ b/src/Navbar.js
@@ -1,113 +1,109 @@
-import React, { Component, Fragment, Children } from 'react';
+import React, {
+  Fragment,
+  Children,
+  cloneElement,
+  useRef,
+  useEffect
+} from 'react';
 import PropTypes from 'prop-types';
 import cx from 'classnames';
 import Icon from './Icon';
 import SearchForm from './SearchForm';
 
-class Navbar extends Component {
-  componentDidMount() {
-    const { options } = this.props;
-
-    if (typeof M !== 'undefined') {
-      this.instance = M.Sidenav.init(this._sidenav, options);
-    }
-  }
-
-  componentWillUnmount() {
-    if (this.instance) {
-      this.instance.destroy();
-    }
+const Navbar = ({
+  id,
+  children,
+  brand,
+  className,
+  extendWith,
+  fixed,
+  alignLinks,
+  centerLogo,
+  search,
+  menuIcon,
+  sidenav,
+  options,
+  ...props
+}) => {
+  const _sidenav = useRef(null);
+
+  useEffect(() => {
+    const instance = M.Sidenav.init(_sidenav.current, options);
+
+    return () => {
+      instance && instance.destroy();
+    };
+  }, [options]);
+
+  const brandClasses = cx({
+    'brand-logo': true,
+    center: centerLogo
+  });
+
+  const navCSS = cx({ 'nav-extended': extendWith }, className);
+
+  const navMobileCSS = cx('hide-on-med-and-down', [alignLinks]);
+
+  const links = Children.map(children, (link, index) => (
+    <li key={index}>{link}</li>
+  ));
+
+  const sidenavLinks = sidenav
+    ? sidenav
+    : Children.map(children, (link, index) => {
+        const clonedLink =
+          link && link.props && link.props.id
+            ? cloneElement(link, {
+                ...link.props,
+                id: `sidenav-${link.props.id}`
+              })
+            : link;
+        return <li key={index}>{clonedLink}</li>;
+      });
+
+  let navbar = (
+    <nav className={navCSS} {...props}>
+      <div className="nav-wrapper">
+        {search ? (
+          <SearchForm />
+        ) : (
+          <Fragment>
+            {brand &&
+              cloneElement(brand, {
+                className: cx(brand.props.className, brandClasses)
+              })}
+            <a href="#!" data-target={id} className="sidenav-trigger">
+              {menuIcon}
+            </a>
+            <ul className={navMobileCSS}>{links}</ul>
+          </Fragment>
+        )}
+      </div>
+      {extendWith && <div className="nav-content">{extendWith}</div>}
+    </nav>
+  );
+
+  if (fixed) {
+    navbar = <div className="navbar-fixed">{navbar}</div>;
   }
 
-  render() {
-    const {
-      children,
-      brand,
-      className,
-      extendWith,
-      fixed,
-      alignLinks,
-      centerLogo,
-      centerChildren,
-      search,
-      menuIcon,
-      sidenav
-    } = this.props;
-
-    const brandClasses = cx({
-      'brand-logo': true,
-      right: alignLinks == 'left',
-      center: centerLogo
-    });
-
-    const navCSS = cx({ 'nav-extended': extendWith }, className);
-
-    const navMobileCSS = cx('hide-on-med-and-down', [alignLinks]);
-
-    const links = Children.map(children, (link, index) => (
-      <li key={index}>{link}</li>
-    ));
+  return (
+    <Fragment>
+      {navbar}
 
-    const sidenavLinks = sidenav
-      ? sidenav
-      : Children.map(children, (link, index) => {
-          const clonedLink =
-            link && link.props && link.props.id
-              ? React.cloneElement(link, {
-                  ...link.props,
-                  id: `sidenav-${link.props.id}`
-                })
-              : link;
-          return <li key={index}>{clonedLink}</li>;
-        });
-
-    let navbar = (
-      <nav className={navCSS}>
-        <div
-          className={centerChildren ? 'nav-wrapper container' : 'nav-wrapper'}
-        >
-          {search ? (
-            <SearchForm />
-          ) : (
-            <Fragment>
-              {brand &&
-                React.cloneElement(brand, {
-                  className: cx(brand.props.className, brandClasses)
-                })}
-              <a href="#!" data-target="mobile-nav" className="sidenav-trigger">
-                {menuIcon}
-              </a>
-              <ul className={navMobileCSS}>{links}</ul>
-            </Fragment>
-          )}
-        </div>
-        {extendWith && <div className="nav-content">{extendWith}</div>}
-      </nav>
-    );
-
-    if (fixed) {
-      navbar = <div className="navbar-fixed">{navbar}</div>;
-    }
-
-    return (
-      <Fragment>
-        {navbar}
-
-        <ul
-          id="mobile-nav"
-          className={cx('sidenav', [alignLinks])}
-          ref={ul => {
-            this._sidenav = ul;
-          }}
-        >
-          {sidenavLinks}
-        </ul>
-      </Fragment>
-    );
-  }
-}
+      <ul id={id} className={cx('sidenav', [alignLinks])} ref={_sidenav}>
+        {sidenavLinks}
+      </ul>
+    </Fragment>
+  );
+};
 
 Navbar.propTypes = {
+  /*
+   * override id
+   * @default 'mobile-nav'
+   */
+  id: PropTypes.string,
   brand: PropTypes.node,
   children: PropTypes.node,
   className: PropTypes.string,
@@ -162,6 +158,7 @@ Navbar.propTypes = {
 };
 
 Navbar.defaultProps = {
+  id: 'mobile-nav',
   options: {
     edge: 'left',
     draggable: true,


Test patch:
diff --git a/test/Navbar.spec.js b/test/Navbar.spec.js
index 5df41bb..6a48e89 100644
--- a/test/Navbar.spec.js
+++ b/test/Navbar.spec.js
@@ -115,6 +115,11 @@ describe('<Navbar />', () => {
     expect(wrapper.find('.breadcrumb')).toHaveLength(3);
   });
 
+  test('can have custom id', () => {
+    wrapper = shallow(<Navbar id="custom-id" />);
+    expect(wrapper).toMatchSnapshot();
+  });
+
   test('can have children with custom ids', () => {
     wrapper = shallow(
       <Navbar>
diff --git a/test/__snapshots__/Navbar.spec.js.snap b/test/__snapshots__/Navbar.spec.js.snap
index 73eb872..e8bdd60 100644
--- a/test/__snapshots__/Navbar.spec.js.snap
+++ b/test/__snapshots__/Navbar.spec.js.snap
@@ -9,6 +9,7 @@ exports[`<Navbar /> adds a brand node 1`] = `
       I AM BRAND
     </span>
   }
+  id="mobile-nav"
   menuIcon={
     <Icon>
       menu
@@ -327,6 +328,35 @@ exports[`<Navbar /> can have children with custom ids 1`] = `
 </Fragment>
 `;
 
+exports[`<Navbar /> can have custom id 1`] = `
+<Fragment>
+  <nav
+    className=""
+  >
+    <div
+      className="nav-wrapper"
+    >
+      <a
+        className="sidenav-trigger"
+        data-target="custom-id"
+        href="#!"
+      >
+        <Icon>
+          menu
+        </Icon>
+      </a>
+      <ul
+        className="hide-on-med-and-down"
+      />
+    </div>
+  </nav>
+  <ul
+    className="sidenav"
+    id="custom-id"
+  />
+</Fragment>
+`;
+
 exports[`<Navbar /> can have custom sidenav 1`] = `
 <Fragment>
   <nav