The last time I had a blog, I started an entry series called “How Not to Write JavaScript”. At the time, I thought it would be useful to showcase how I have refactored code that I believed was over-engineered, poorly written, outdated, all of the above, etc. I realize that I might have been a little harsh in the way that I went about it, so I didn’t migrate those entries when I moved to this blog. However, I still see certain blocks of code that I would love to share how I would improve them. I don’t want to do so in an accusatory or shaming way; hopefully I can do so in a way that is informative. That way, if a person who writes something like this eventually sees this blog entry, they can learn something rather than feeling ashamed.

Code

const [ isHovered, setIsHovered ] = useState( false );
return (
  <div
    className={cls( styles.thing, { [styles.thingHover]: isHovered })}
    onClick={onClick}
    onMouseEnter={() => setIsHovered( true )}
    onMouseLeave={() => setIsHovered( false )}
  >
    {thing.text}
  </div>
);

At first glance, you can say that this person understands React and the code looks consistent and clean. They also basically understand how to use event callbacks combined with basic React state management, JSX, CSS modules, and the classnames NPM package. However, it appears that this person didn’t fully understand the way that HTML and CSS natively work without frameworks or programming for accessibility. (Equal responsibility goes to whomever approved it in code review.)

I’ve seen a bunch of blocks like this in the wild in the last several years, including in an application that overrode basic link functionality for a web app by changing the window location manually. These are both excellent examples of over-engineering in front end development.

Objective

After reviewing the code in question, I went hunting to see if this code was being used to set any values elsewhere, and it wasn’t. It’s one thing if this also had to change a value or state somewhere else in the application, but all it was doing was changing styles on hover by adding or removing the thingHover class.

Resolution

I switched this out for a CSS :hover pseudo-class in order to make it simpler, and use more of what is available in the native browser and less JavaScript. Essentially:

<button onClick={clickHandler} className={styles.thing}>
  {thing.text}
</button>

.thing {
	/* Original styles */
}

.thing:hover {
	/* Hover styles */
}

If some other component in JavaScript or React needs to know when a user is hovering on this component (for whatever reason) and it doesn’t just change styles, then the onMouseEnter and onMouseLeave events are more useful. In this case, the former code block was unnecessarily rewriting the browser’s interpretation of a user hovering on an item and adding complexity to the existing codebase that we shouldn’t have to maintain. Choosing to roll your own version of any native browser functionality can be a recipe for disaster in the long run. Instead of relying on functionality maintained by browser developers and well-defined industry standards, the former might be completely ignored, break over time and cause accessibility issues. Changing an element or child element’s styles based on hover is something that should be done using the CSS :hover pseudo-class, which has been a staple of web development for the last several years, and is unlikely to change anytime soon.