3
\$\begingroup\$

I'm new to react and am building a basic react app and have just built a basic area where navigation links render on a page but wonder whether my current start could be optimised - for example, I wonder whether an area like this:

render() {
    return (
        <Root navLinks={this.state.navLinks}></Root>
    );
  } 

Could be changed to something like this:

render() {
    return (
        <Root>
           <NavLinks navLinks={this.state.navLinks} />
        </Root>
    );
  } 

And I wonder about other aspects of the app such as do I need an App.js file or index.js file and could have one rather than the other?

Any quick basic beginner advice would be much appreciated. The code can be better found in this Stackblitz demo or below:

App.js:

import React, {Component} from "react";
import {navLinks} from "./components/nav-links";
import Root from "./components/Root";

export default class App extends Component {
  constructor() {
    super();

    this.state = {
      navLinks: navLinks
    }
  }

  render() {
    return (
        <Root navLinks={this.state.navLinks}></Root>
    );
  } 
}

index.js:

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

ReactDOM.render(
  <App />,
  document.getElementById('root')
);

/components/Root.js:

import React from "react";
import NavMenu from "./nav-menu";
import BodyCode from "./body-code";

export default class Root extends React.Component {
    render() {
        return (
            <div id="container">
                <div id="header">
                    <NavMenu navLinks={this.props.navLinks} />
                </div>
                <div id="body">
                    <BodyCode />
                </div>
            </div>
        )
    }
}

/components/body-code.js:

import React from "react";

export default class bodyCode extends React.Component {
    constructor(props) {
        super(props);
        this.state = {isToggleOn: true}; // boolean for event listener alternative
    
        // This binding is necessary to make `this` work in the callback
        this.handleClick = this.handleClick.bind(this);
      } // bind is saying make the ‘this’ keyword bind(this), where the 2nd ‘this’ is the ‘this’ of the constructor
    
      handleClick() {
        this.setState(state => ({
          isToggleOn: !state.isToggleOn // sets state to the opposite of whatever it is
        }));
      }
      
  render() {
    return (
      <div>
          <p>some body text</p>
          <button onClick={this.handleClick}>
            {this.state.isToggleOn ? 'ON' : 'OFF'}
            </button>
      </div>
    )
  }
}

/components/nav-links.js:

export const navLinks = [
  {
    name: "home",
    href: "/"
  },
  {
    name: "subs",
    href: "/subs"
  }
];

/components/nav-menu.js:

import React from "react";

export default class navBar extends React.Component {
  render() {
    return (
      this.props.navLinks.map((link) => <a href={link.href} key = {link.href} > {link.name} </a>)
    )
  }
}
\$\endgroup\$
2
\$\begingroup\$

Questions

Should Root take props or wrap children components?

<Root navLinks={this.state.navLinks}></Root>

vs

<Root>
  <NavLinks navLinks={this.state.navLinks} />
</Root>

I think this primarily comes down to the purpose of Root. If it is a general purpose wrapper container component, then the latter pattern makes sense. I.E. it is providing the layout but no actual UI, this works well. If on the other hand it isn't very generalizable, i.e. it is more single purpose, then passing it props is preferred. I understand this doesn't sound like an answer, but like many things in javascript, there's no clear, hard line between them and this could also simply come down to personal preference/opinion.

Do I need an App.js file or index.js file and could have one rather than the other?

Again, these both exist by convention, but you could easily merge the two if you so desired. The distinction basically comes down to separation of concerns. index.js is primarily concerned with rendering the application into the DOM whereas App.js is primarily concerned with being the root application container. This is typically where redux stores & providers, react contexts & providers (well, all "providers" in general), root routers/navigation, theming, etc, etc... would/could reside and provide functionality to the app.

Code Review

App.js

  • For such simple components (no state updates or no state, no lifecycle functions, etc..) it is recommended to instead use functional components
  • There is no compelling reason to store the static link data in react state, just pass it directly to props
  • navLinks isn't a component so it probably shouldn't be stored in that directory, better suited for a utility directory or similar
  • Root doesn't wrap any children components so it can be self-closed

Suggestion

import React from "react";
import { navLinks } from "./components/nav-links";
import Root from "./components/Root";

export const App = () => <Root navLinks={navLinks} />;

/components/Root.js

  • Many of the same comments as for App.js
  • If the element ids are actually used (CSS, testing) then keep them, otherwise they just clutter the JSX
  • BodyCode is already wrapped in a div, so the div here is extraneous and helps increase clutter in the DOM

Suggestion

import React from "react";
import NavMenu from "./nav-menu";
import BodyCode from "./body-code";

export const Root = ({ navLinks }) => (
  <div>
    <div>
      <NavMenu navLinks={navLinks} />
    </div>
    <BodyCode />
  </div>
);

/components/body-code.js

  • By convention react component names are PascalCased
  • handleClick can be defined as an arrow function, which would make binding this to it in the constructor unnecessary
  • Great job on using a functional state update to toggle the isToggleOn state value
  • While not a necessity, the constructor can be removed and state defined as a class property
  • Also not a necessity, this component is still simple enough and doesn't utilize any lifecycle functions that it is also a good candidate for conversion to functional component

Class-based suggestion

import React, { Component } from "react";

export class BodyCode extends Component {
  state = {
    isToggleOn: true,
  };

  handleClick = () => this.setState(
    prevState => ({ isToggleOn: !prevState.isToggleOn })
  );

  render() {
    return (
      <div>
        <p>some body text</p>
        <button onClick={this.handleClick}>
          {this.state.isToggleOn ? 'ON' : 'OFF'}
        </button>
      </div>
    )
  }
}

Functional component suggestion

import React, { useState } from "react";

export const BodyCode = () => {
  const [isToggleOn, setIsToggleOn] = useState(true);

  const handleClick = () => setIsToggleOn(on => !on);

  return (
    <div>
      <p>some body text</p>
      <button onClick={handleClick}>
        {isToggleOn ? 'ON' : 'OFF'}
      </button>
    </div>
  );
};

/components/nav-menu.js

  • Should follow react component naming convention
  • Simple and should probably be converted to functional component

Suggestion

import React from "react";

export const NavBar = ({ navLinks }) => navLinks.map(link => (
  <a key={link.href} href={link.href}>
    {link.name}
  </a>
));

Other Suggestions

Use a routing/navigation solution versus using anchor (<a />) tags as these actually reload the app. These typically provide all the functionality of links and navigating around your app by manipulating the browser's history stack but won't reload the page between navigations.

react-router/react-router-dom is a common routing solution. The idea being you define Routes, wrapped in a Router/Switch, to render each "page" of your app, and instead of anchor tags a Link is used to go to a specified path.

A sample router in BodyCode

import { BrowserRouter as Router, Route } from 'react-router-dom';

...

<div>
  <p>some body text</p>
  <button onClick={this.handleClick}>
    {isToggleOn ? 'ON' : 'OFF'}
  </button>
  <Router>
    <Route path="/subs" component={Subs} />
    <Route path="/" component={Home} />
  </Router>
</div>

NavMenu

import { Link } from 'react-router-dom';

...

navLinks.map(link => (
  <Link key={link.href} to={link.href}>
    {link.name}
  </Link>
))
\$\endgroup\$
  • 1
    \$\begingroup\$ wow, thank you so much! excellent and VERY helpful response. I think a key takeaway as a beginner is knowing when to use functional vs class components as you seem logically correct. And using arrow functions for brevity seems like another nice touch. Thank you very much :) \$\endgroup\$ – user8758206 Sep 1 '20 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.