Some Frontend Code Review Tips

Preface

We are in an odd era of frontend development. It seems as if the pendulum of complexity has swung our way. The abstractions we use are heavily opinionated. The platform we deliver on changes week to week with new features. Languages such as CSS and JavaScript seem full of pitfalls, gotchas, and snake pits.

This is particularly stressful for developers who aren’t too familiar with the intricacies of the frontend. The goal of this document is to answer the question: when you’re tasked with a frontend code review, what can you look out for?

A Word on React

Until very recently, React best practices felt almost like tribal knowledge. Textbooks would be quickly outdated with newly discovered practices or design patterns, or just the shifting nature of the web and user needs that needed to be met. Packages to help us do something were there one day and gone the next. API changes were not always explained well enough, and full stack developers who have a whole other swathe of problems to deal with don’t have the time to discern the intricacies of “thinking in React” when it requires watching a talk, or reading a GitHub issue somewhere.

The old React docs were very much outdated and lacking direction. The new docs over at react.dev are more thorough and well written, including interactive examples and solutions to common problems.

Most of the React information in here should be referenceable on react.dev and will be linked in the section where possible. Otherwise I will try to find the other sources I pull from. Some of the tips will just be very broad, others will be specific examples of things I see and frequently comment on in React code.

The Low-Hanging Fruit

Ask Yourself:

  • ✅ Does this need to be a useEffect?
  • ✅ Can this be done in CSS?

Does this need to be a useEffect?

The answer is probably no. Here’s a bit of history. useEffect since its addition to the React API has been the most misused function, and it’s not the developer’s fault. The React team just never explained well enough how they intended it to be used until recently. In fact, there were non-core React developers out there doing whatever the opposite of evangelizing is (demonizing?) for useEffect. It has rightfully earned the ire of many developers out there, especially because its misuse can directly lead to bugs.

The fact remains however that there are many cases for it, and its understanding is important to React development

useEffect Checklist

  • Does the logic within useEffect synchronize with something outside of React’s internal state? (e.g., an external data source?)
    • Maybe you need to synchronize your form with API values on component mount, and trigger some state update?
  • Can this state be derived?
  • Is this logic tied to an event?
    • If it is, the execution should be part of an event handler instead of being triggered by a change in dependencies to useEffect

Everything in this checklist can be found in greater detail in the Resources section.

When would we need an effect?

A good question! At it’s simplest, its when you need to synchronize with external state. It’s a vague term for sure, but think of it this way

  • Do you need to fetch data on component load?
    • You can plop a useEffect in there and invoke fetch(). The caveat here is if you’re building a client side application and not using React Query or RTK Query or something, I’d strongly suggest it.
  • There’s some cleanup required on component unmounting
    • There may be a component that fires an event on unmounting. This happens especially when using React inside a pre-existing enterprise application.
  • Attach a global listener of some sorts.
    • This one is easy, if you need to attach a listener to window or some element that you do NOT render in your application lifecycle. For example, adding click handlers to close popovers when the click lands outside the containing element.

Example 1: Data Fetching

import { useEffect, useState } from 'react'
import { getUserData } from '@api'

function App({ children }) {
	const [data, setData] = useState()
	useEffect(() => {
		getUserData().then(
			_data => setData(_data)
		);
	}, [])
	return (
		{
			data && data.map(
				user => <User key={`${user.id}`} user={user}/>
			)
		}
	)
}

Example 2: Cleanup

function App() {
  const [isSaved, setIsSaved] = useState(false);
  const unloadListener = (event) => {
    event.preventDefault();
    if (!isSaved) {
      return (event.returnValue = "");
    }
  };
  useEffect(() => {
    window.addEventListener("beforeunload", unloadListener);
    return () => window.removeEventListener("beforeunload", unloadListener);
  }, []);
}

Example 3: Global Listener

function useClickOutside(ref, clickHandler) {
  const listener = (event) => {
    if (element && !element.contains(event.target)) {
      clickHandler(event);
    }
  };
  useEffect(() => {
    const element = ref.current;
    document.addEventListener("click", listener);
    return () => document.removeEventListener("click", listener);
  }, []);
}

useEffect Resources

Can This Be Done in CSS?

This one is one that can be hard to check if you’re unfamiliar with CSS, and the end results may be the same. Thus, it gets the “let’s not poke the sleeping bear” treatment. “Good enough” is often times “good enough.”

However, with a little bit of CSS you can actually reduce your component logic, sometimes eliminating entire props or even components. And when you start to go down the path of realizing how much power CSS gives you over styling depending on behavior, you can really start to deliver quality user experiences with less mental overhead.

Don’t ask “how do I get this class name to appear correctly given the circumstances?” and instead focus on “do I have the correct document structure?” and let CSS take care of the rest.

The CSS Checklist

  • Should this be part of the rendering logic because it has an effect on the document structure, or is this purely aesthetic?
  • Does this element need a specific class name, or can it be targeted with a reasonable selector?
  • Is there an attribute for this?
    • Admittedly, this is hard. But try to make sure there’s a more correct way to do this styling and reach out if you’re unsure.

Consider: the horizontal separator

We’re all big fans of <hr/>, it’s a personal favorite HTML element (top 5 I’d say, after maybe the <p/> tag).

The thing is though, if you’re just trying to draw a line between some components (say a horizontal line between some sections) consider just using CSS. Don’t take it from me, Mozilla Developer Networks suggests as much. Here’s an example

đŸš« Do not do this

/** A component that wraps your content into sections */
function Section({ isLastSection, children }) {
	return (
		<section class={`section ${isLastSection ? 'lastSection' : ''}`}>
			{children}
		</section>
	)
}

function App({ data }) {
	return (
		<div className="app">
		{data.map((stuff, index)) => (
			<Section isLastSection={index === stuff.length - 1}>
				<h2 className={`${index === 0 ? : 'first-header' : ''}`}>
					{stuff.heading}
				<h2>
				{stuff.content}
			</Section>
		)}
		</div>
	)
}

It’s a seemingly easy block of code to what is considered a simple problem. But not only is it more overhead (imagine this with types), but it is not the idiomatic approach to frontend development.

CSS has many selectors for dealing with quantity, type, state, and other aspects you’d be surprised to know (with plenty more on the way in 2023-2024).

✅ Do something like this

section:not(:first-child) > h2 {
  padding-top: 8px;
}

section:not(:last-of-type) {
  border-bottom: 1px solid grey;
  padding-bottom: 16px;
}

Further Examples

Empty Stuff

Imagine there is a <table/> which has cells that may receive empty content

function PriceCell({ price }) {
  let displayPrice = "";
  if (price) {
    displayPrice = `$${price}`; // e.g., $5
  }
  return <td>{displayPrice}</td>;
}

Instead of say <td>{price ?? '-'}</td>, just use the :empty pseudo class

td:empty::before {
  content: "-";
}
Spacing

If you have a row of stuff that you need separated by some content: look out for this common CSS usage. gap is great for spacing between elements. Margin does fine when supporting older browsers however.

.row-of-things {
  display: flex;
}
/* being applied to all items via JS except the first one */
.row-item:not(:first-of-type) {
  margin-left: 8px;
}

Just do this instead:

.row-of-things {
  display: flex;
  gap: 8px;
}

CSS Resources

It is hard, because essentially this section tries to encapsulate that certain things can be done in CSS. And the links would be
 everything you could do in CSS. But instead I’ll leave some nice selectors that I think can remove some rendering logic you use.

  • :has - An extremely powerful CSS selector that allows you to query descendants and can be chained with other selectors.
  • :is - If you can have content that is displayed but different types of elements, you can use the :is selector to make determinations on the styling
  • web.dev showcases some really neat design patterns and has a lot of great CSS walkthroughs.
  • For more digestible tips and tricks, follow CSSWG developer advocates like @una and @adamargyle from the Chrome Dev team

More General Advice

”Use the Platform”

This quote is the mantra of Remix, a metaframework for React. In their core philosophies, they talk a lot about using the native browser behavior when possible to exhibit behavior. It will result is a better developer experience due to the discoverability of built-in methods and operations. It’s also often the most optimized and direct way to accomplish something. Not everything can be done natively, or is necessarily “always the best.” I’ll give an example though of how certain problems are already solved, and don’t need a lot of over-engineering.

If you ever come across some problem and are unsure how you might go about doing it in React, first try to figure out how you’d do it without React! It’ll give you a better understanding of the problem and its available solutions.

Practical Example: Scroll Into View

Let’s say that you build a workflow that has a “Next” button at the top right. When you click this “Next” button, it’ll find the form field with an error and scroll to it. How might you go about doing that?

function SignupForm() {
  return (
    <div className="signup-form">
      <Form>
        <Button type="submit">Next</Button>
        <Input required name="firstName" />
      </Form>
    </div>
  );
}

Your instinct might be to create a ref, and pass that ref to the Input component so that it can be forwarded to the underlying <input/> element. Then inside some submit handler check if the ref.current.value is not empty, and after which you’ll window.scroll to some x,y coordinates?

Now imagine if you have 20 fields in this signup form. I know, really imagine. Would you store all the refs for all the input fields?? And do some switch/case action? Maybe, but it seems like an indirect way to solve a very common behavioral pattern we see on the web (telling us which field is invalid and scrolling to it).

Well, you can just use the onInvalid event, and the scrollIntoView!

function SignupForm() {
  return (
    <div className="signup-form">
      <form
        onInvalid={(e) => {
          e.target.scrollIntoView({
            behavior: "smooth",
            block: "nearest",
            inline: "center",
          });
        }}
      >
        <button type="submit">Next</button>
        <label>
          First Name:
          <input required name="firstName" />
        </label>
      </form>
    </div>
  );
}

The <form> element captures all the errors and changes happening to any of its child input fields. The target property is the input that caused the event. In this case, it’s the input which was required that has not been filled out. You can then just use the scrollIntoView API to scroll to the element, with some arguments on how you want the scroll to behave.

All this to say, look for native solutions where you can. Obviously there are React-y ways to do things. Those are mainly principled ideas such as “keeping your components pure” and “keep state DRY” and “render shouldn’t have side effects” etc. Or it can be more tangible things like “this should be more declarative.”

Identifying when a solution is overcomplicated is a tough one! In order to help with that, ask yourself if what you’re looking at is a common behavior or occurrence across the web. If there is a web standard way to do this, it’ll probably be the least frictional.

Author’s note: The “use the platform” quote has been memed into oblivion because it brought some weirdos out of the woodwork suggesting that frameworks are pointless because you can do everything with raw HTML and plain JS. It bears stating that I am not one of those.

I love my frameworks, and I dislike rediscovering solved problems. The loudest voices in the Web Dev world can be some tech dudes who want to engagement farm, which leads to things like “react is dead, just use solidjs, vue, lit, preact, svelte, htmx” etc., etc., etc.

The browser does a lot of great stuff, yes. But users do not only desire what the browser can do natively, it’s why the WHATWG and TC39 exist, because there are unmet needs somewhere out there.

Writing Tests

We all like writing tests
 right?

No, of course not. getBy, findBy, queryBy?? What do you mean “should be wrapped in act(
)“?? I’m just going to container.querySelector('.my-long-bem-classname') and assert on stuff!

Don’t do it, though. The temptation is there to circumvent Testing Library when you can to assert on things, but it is not advised. Testing Library is a very sound library, and so many frameworks have invented wrappers around it not because it’s the only test runner, but because it does the best job of enforcing good frontend principles. Proper attributes, labeling, markup structure, etc. If you find yourself fighting Testing Framework, you’re probably actually doing something wrong and should carefully consider the errors before circumventing them!

Here’s a quick checklist for things you should look out for when reviewing or writing tests:

  • Is the test doing some interactivity that changes state, and not wrapping in act() ?
  • Does the test try to access values of input fields programmatically? As in, by querying for the element and asserting expect(element.value).toBe()?
    • Testing Library has APIs to handle input fields and form values for you. Check out .toHaveFormValues , it makes testing forms a lot easier. It will also enforce that your markup has the proper labels and names associated with the inputs.
  • Does this element need a data-testid or is there a built-in way to get this element?
    • *byRole is underrated. Check what the native role of the element you’re looking for is. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles
    • *byText looks for elements matching the text you provided against the textContent of an element. Even if a header element has a child span for example, you can use *byText to find that whole header and all of its text due to the normalization.
  • Are we awaiting on the proper states?
    • If your component has some async behavior, you can just use screen.findBy methods and await on their response. findBy and findAllBy have a waitFor built into them, and so you don’t need to wrap them. More on that here
    • If after the initial data load there is no more re-rendering, you can just use getBy after the first findBy

For a more exhaustive list from the proverbial horse’s mouth, check out Common Mistakes with React Testing Library by Kent C. Dodds

That’s it for now

Thanks for reading! I hope you gained something from this. Feel free to leave feedback and request for clarifications on things!