The Designer's Checklist for Reviewing Pull Requests

Lela Kodai by Lela Kodai

The Designer's Checklist for Reviewing Pull Requests

Lela Kodai This checklist is meant as a guide to help you keep track of everything going on in a pull request. Use this list as a way to feel confident that you’re approving high-quality work.

posted in Design on July 25, 2017 by Lela Kodai

Imagine this: you’re a designer who has been tagged on a pull request as a reviewer. It’s up to you to look at the changes made and decide if you are going to approve them and let them frolic with the rest of the app code.

If your team is anything like the teams with which I’ve worked, you’re the only one really scrutinizing the styles and html. And if you’re anything like me, you don’t like to add your name to something unless you’re confident it’s good work. So how do you figure out if this PR is any good?

While bringing a new hire up to speed, I was explaining some of my process and told him I have a mental checklist. The obvious thing to do was to turn my mental checklist into an actual checklist so I could share it. Bitovi LOVES checklists, so now you can download a pdf of Lela’s “designers can code some too” PR Checklist, to get some help covering all your bases. And if you stick around, the rest of this article will go into a bit more detail about it. 

Lelas-PR-Checklist.jpg

Click the image above to download a PDF version

Break it down now

The checklist is broken into two main sections:

  1. “The Diff” in which you look at the comparison between the pull request’s changes and how the application already works.
  2. “The App” in which you actually look at the changes being reviewed locally on your machine.

The Diff

All the items in “The Diff” are to help ensure the code is well-written, maintainable, and usable (or at least can be understood) by other team members. Code written without thought for semantics, style, previously done work, or modern tools make an application an increasing nightmare to work on.

Make sure the code in the pull request uses your UI framework as expected, and uses new layout tools (like CSS Grid) accurately. When I wrote this list originally, we were using Flexbox in production for the first time, so remembering to double-check all flex attributes was especially key.

Also including documentation can be paramount to maintaining the application. If you’re not following the Style Guide Driven Development model of writing html/css, then at least include some code comments on more complex pieces. It can save you a lot of “wtf is this??” troubleshooting later. 

More specifically here's a rundown of each item in The Diff:

  • LESS/SASS variable usage: Are variables being used generously and appropriately?
  • Semantic HTML (headings in order, tags used appropriately): Is HTML being used judiciously and appropriately?
  • Accessibility (alt tags, labels for inputs, table summaries): Are accessibility guidelines being followed in the markup?
  • Markup patterns used consistently: Consistent code is easier to read, maintain, and reuse.
  • Any duplicate CSS that has been created elsewhere already: Does code need to be made more global or put into a mixin for better maintainability?
  • Flexbox/Grid usage: Can floats and hacks be avoided with the use of newer and better layout styles?
  • Code formatting/style according to the team’s guidelines: Make friends with developers and keep code neat by sharing indent sizes and other minor things (do you put the unit after a 0? Alphabetize attributes? Use px or em or rem or %?)
  • Documentation: Does it exist? Does it make sense?
  • Typos: Loose lips sink ship(ments of applications to production server)s.

The App

All the items in “The App” are to help ensure the code is taking the project forward, and not in circles (or worse: backwards!). Actually running the code can enable you to catch weirdness or errors that would have otherwise been impossible to see by just reading the source code. I’ve been writing CSS for around 15 years, but “The icon in that button isn’t centered” is not information I can usually glean from a code diff.

The other most important thing running the code can help reveal is bug regressions. Sometimes editing CSS can feel like this gif:

css-wood-puzzle.gif  

It’s much easier to see if one bug is going to replace another if you’re looking at the actual app.

More specifically, here's a rundown of each item in The App:

  • Actual expected functionality: Does the branch actually work in browser?
  • Parity with the designs/discussed changes: Does the app look how it should?
  • Responsive transitions and awkward places: Do things get weird at various sizes? (Especially check tablet ranges!)
  • Rendering bugs in various supported browsers: Does it look broken in any browser? (Looking at you Safari)
  • Regressions: Does it unfix anything?
  • Edge use cases (error/empty states, long names, data variation etc.): Is there error handling? Empty messages? Does text wrap without breaking the layout?
  • Logic: Does it make sense? Does micro copy need to be adjusted?
  • Typos: Loose lips sink ship(ments of applications to production server)s.

Gain Confidence

This checklist is meant as a guide to help you keep track of everything going on and to feel more confident that you’re approving high-quality work. If you haven’t already, download my checklist: Lela’s “designers can code some too” PR Checklist  or feel free to make your own checklist. Any list will help you keep track of all the moving pieces.

Bonus Hint: Use this checklist before submitting your own pull request to feel good about the code you’re presenting to coworkers.

Create better web applications. We’ll help. Let’s work together.