Skip to content

Code reviews

Jump to bottom
ashleykolodziej edited this page Jan 7, 2019 · 1 revision

In this document, we'll cover:

Why do we do code reviews?

  • They make future upkeep easier by ensuring everything adheres to our coding standards
  • They can catch potential issues before a client sees them
  • They're a great way for you to learn how you can improve your code next time

Guidelines for Contributors

  • You can open a pull request at any time, but please wait to assign a reviewer to your request until you're ready for feedback.
  • Try to keep pull requests small - fewer than 400 lines changed if possible.
  • You can ask for feedback at any time, and you can continue working while the review is going on as long as your changes are small - things like bug or CodeClimate fixes. Don't add new functionality after assigning someone to review.
  • If you need to keep working while waiting for review and you're dependent on that branch to move on, it's okay to make a new branch off the branch that's currently in review.
  • Work to resolve CodeClimate issues where possible, but don't go out of your way to resolve issues that are impossible or don't make sense. The reviewer will manually approve issues that you can't resolve.
  • If you see a CodeClimate issue that isn't your fault due to WordPress standard, an outside API, Responsive templates, or any other situation that's out of your control, explain the situation and ask your reviewer to manually approve.
  • Code reviews can proactively catch bugs, but ultimately it is your responsibility to test your code. When you merge into master, especially on a site that's already launched, that is your promise that you've tested your code thoroughly and it is bug-free and ready for production use. Reviewers are not responsible for testing your functionality.

Guidelines for Reviewers

  • Code reviews are very time sensitive and often close to deadlines. Do your best to do the review within the same day or find someone else who is able to review in a timely manner if you can't.
  • Code reviews are meant to be relatively quick, but don't rush them. Reviews tend to take anywhere from 5-60 minutes, with most reviews taking around 15 minutes. If your review is taking more than an hour, talk with the designer or developer about reducing the size of their pull request.
  • Watch for things a computer can't catch - better ways to do things, things going off standard, and potential bugs or security issues.
  • Do your best to get your feedback in a single review. Don't request changes multiple times unless absolutely necessary.
  • Be mindful of times where code is out of the developer's control, and manually approve CodeClimate issues where necessary. If the issues are extensive, but entirely due to outside factors, don't worry about approving individual issues, just approve the entire thing.
  • When a change is critical and can't go out to a production site, mark your review as "Request changes".
  • If you know the UI or meta name has to change due to a client need, mark your review as "Request changes" so the client does not begin entering content into UI that may change and lose their data.
  • If you are uncomfortable with a change going out as is, and neither of the above rules are true, consider approving with comments, but encouraging the author to open a new pull request with your suggested changes. You can file an issue as well so they don't forget and link to your original feedback. Always check with the author on deadline and timing before marking "Request changes" on this type of feedback.
  • If the code is functional, but there are small style improvements or suggestions for next time, mark your review as "Approved" and leave comments so the author can tackle as many of the improvements as are reasonable for their current timeline.
  • If you are happy with everything, mark your review as "Approved". Never leave a comments-only review without explicitly approving or requesting changes.
  • It's not required for you to review code outside your realm of expertise, but it's encouraged for you to take a quick look if you have time, especially if it's code that affects your review.

What to expect during your code review

Each code review will be slightly different due to varying project timelines and budgets. If you have any critical items, you should fix those as soon as possible in your theme. You may also have some suggestions, or topics to look at if you run into a similar problem later. These are changes that you can choose to incorporate or ignore at your own discretion. While you may choose not to deal with these items in the theme you're working on now, it's highly recommended you keep them in mind and give them a try next time.


# Code review checklist

This is the checklist we use to do code reviews. If you keep it in mind as you build your theme, you'll have fewer items to fix at the end of the project.

Content within theme

  • Images are properly named
  • Images have been properly sized and compressed for web
  • All assets use https (no http!)

Sass

  • Code follows Responsive Framework coding standards for Sass
  • Making sure to use Sass placeholders instead of includes
  • Utilizes built in classes (col-md-6, col-md-half) instead of hand-coding widths
  • Utilizes mixins, includes, and variables properly
  • Code is readable, clear, and uses common patterns

Formatting and order

Tip: You may be able to automate formatting in your code editor. CSSComb is one tool that allows you to do this, and is available for Sublime Text 2. These handy JSON settings will cover all code formatting needs except for grouping @extend and @include and comment formatting.

There appears to be a Coda plugin for CSSComb as well. This hasn't been tested in our environment - explore at your own risk.

Naming and specificity

Variables, Placeholders, and Mixins

  • z-index is managed through variables
  • Code uses variables appropriately
  • Code uses placeholder classes (@extend) appropriately (static repeating CSS rules such as clearfix apply here)
  • Code uses mixins (@include) appropriately (dynamic repeating CSS rules which need flexibility to take a variable, such as transition, apply here)

Overall principles

  • Code is readable and easy to understand
  • Code is clearly organized, with like rules grouped together
  • Code is divided into well-named partials that make sense for the project needs
  • Code effectively uses @extend and @include to reduce repetition and abstract common sets of rules

Whoever is reviewing your code may also choose to make additional suggestions on style, best practices, or architecture at their discretion.

Javascript

PHP

Framework and Plugins

All of the above, plus:

  • CSS generated by plugin or framework uses default responsive breakpoints
  • Code is extensible where designers may want custom functionality
  • Variables are available in the Sass for this feature for colors, fonts, and other CSS properties a designer is likely to want to change
  • All variables that are declared are actually used
  • Code / classes are properly scoped: it should be clear from naming there will be no interference with a theme or other plugin CSS

Welcome to Responsive!

Get started

Configuration

Build child themes

Sass

Javascript

PHP

Shortcodes

Templates

GitHub

Tasks

Contribute to the framework

Code Examples

BU Developer Resources

Clone this wiki locally