Author Archives: Michael Woo

Effective Front-End Code Review

 

Whether it is open-source or in-house code, front-end or back-end code, the pull request (PR) model is widely used in the industry. Before a PR is merged, it is reviewed by peers, and the code is modified by the developer as needed, in an iterative and interactive process. Ideally, we want to spend quality time reviewing each modified line (and any unmodified but associated code that may be impacted) so that we don’t defer and build tech debt when code is added or modified.

However, with pressing timelines, sometimes we simply don’t have enough time for code reviews. And when we do have time, we may not be doing a good job because we don’t have a clear set of team coding guidelines or we may not know what we should look for. Below are some suggestions about how code review can be done, with specific front-end things to focus on.

Code review prerequisites

  • Thorough and meaningful code reviews take time. We need to scope for code review just like any other task. For that to happen, everyone must believe in its purpose and importance — that means management and product managers must actively encourage code reviews and give developers the necessary time.
  • The team has agreed on naming convention, styling, and coding standards, whether it is HTML, CSS, or JS. Styling standards give the code a consistent and professional look. A naming convention makes it easier to understand, search, build, and maintain. Coding standards make the code cleaner and easier to read, build, maintain, and debug. There are many popular standards out there. It is less important which one you choose, but it is crucial that the entire team follows one standard. If a rule is controversial within the team, discuss and decide, then change the standard and apply the new rule across the entire code base. Consistency is key.
  • The team has agreed on what to review — from spaces and camelCase, to where business logic should be, to whether the new code is violating established architecture.

Some things to look for in HTML

  • Does the code follow agreed-upon standards and conventions?
  • Does the markup use the same element ID name more than once? If so, change it to a class because each ID should be unique. If a reusable module contains an element ID, the ID is no longer unique once the module is reused in the same page. Remember that getting an element by ID returns only the first element that matches, which may cause issues when you have multiple elements with the same ID name.
  • Are there unnecessary wrapper elements? If a <div> does not have a class, or the <div> has a class but the class has no associated styles, chances are that it is not needed. Remove it and check the layout. If the layout does not change, you do not need that wrapper.
  • If the app uses a template language that allows complicated logic, are you over-using it, thus making the markup hard to read? Consider applying the logic to the view model BEFORE the view model is passed to the template.
  • Does the markup contain the proper ARIA attributes for accessibility?

Some things to look for in CSS

  • Does the code follow agreed-upon standards and conventions?
  • If you are using a CSS precompiler, such as LESS or SASS, are you nesting classes (or even worse, nesting IDs) because it looks more organized? Unnecessary nesting makes overriding CSS difficult; it also hurts performance and maintainability.
  • Is CSS in modules namespaced to avoid name clashing? Consider a top-level wrapper class with the same name as the module. If you don’t want class nesting, consider using BEM convention.
  • Are some class names so generic that name clashes are likely? This can happen with either other modules in the same app or with other libraries that you may use.
  • Are regularly used CSS rules (or blocks of rules) repeated so often that they should become mixins?
  • If the app uses a CSS library, such as Bootstrap or Skin, are you maximizing its use? Does your app redefine mixins available in the library, or worse, does your app have blocks of CSS where using a single class from the library would do?
  • Are padding and margin used correctly? Is padding used when margin should have been, or vice versa?
  • Does using percentage instead of pixel make sense? How about rem vs. px?
  • Are there duplicate rules for the same selector? Remove the duplicates.
  • Are the same rules found for different selectors in the same module? Merge them.
  • Are the same rules found across different modules? Put them in a shared CSS file.
  • Are there too many nested classes? Are they necessary for override? Try removing some nesting. If the layout doesn’t change, they are not needed.
  • Are IDs used for CSS? IDs are HIGHLY specific and need other IDs to override them. And if you must insist on using IDs for CSS, DO NOT nest IDs. First, nested IDs require the same number of nested IDs or more to override, which makes your CSS longer and more complicated. Second, and this relates to fundamental CSS principle — IDs are by definition unique. If an ID needs to be nested to work, it’s not unique. It should not be an ID. Make it a class instead.
  • If the code uses media queries, see the “Media queries” section in this post.

Some things to look for in JavaScript

  • Does the code follow agreed-upon standards and conventions?
  • Is there JSDoc or similar documentation, especially for exposed methods?
  • If something is to be fixed/enhanced later, is there a TODO with a developer’s name next to it?
  • Is a solution either too clever or too complicated? Is there a slightly longer but simpler and more maintainable solution? If you have to read the code (or its comments) several times to understand what it is trying to do, it is probably too complicated and should be simplified.
  • Does each method do one specific task only? If not, break it up into smaller, focused tasks. It will help in reusability and unit testing.
  • Is a method particular to one use case, with hard-coded values and hard-coded CSS selectors, and so on? Or is it generic and reusable, and takes parameters? Generic methods are scalable and easier to maintain, AND they are much easier to unit test.
  • Does a block of code look familiar from elsewhere? If an existing util method does the same, use it. If there is a util method that is related but slightly different, update it. If the util does not exist, create it. Then, use that util and remove identical and similar blocks globally.
  • If an app already uses a utility library such as Underscore or Lodash, is the PR introducing another similar library? Discuss with your team which library best suits your app.
  • Is the app loading a heavy library when you use only a few methods in it? Is there a lighter alternative? Can you pull out just what you need?
  • Does a function take too many parameters? If there are more than 4 (or some number set by your team), use an object to avoid the hassle of parameter ordering and null values.
  • Are values and data cached? Any string, object, etc., should be cached if it is used more than once.
  • Are variable and function names so short that they are not meaningful at a glance? Longer names (be reasonable, of course) are more likely to be meaningful and self-documenting. Furthermore, searching globally for a longer name will return fewer results, which makes code-digging easier.
  • Is there code that looks like the following? Use Lodash’s get (or similar method from another library) to get an object property down a path. You can avoid long chains (it’s easy to miss a check) and undefined exceptions when the response is not what you expect.
    if (a && a.b && a.b.c && a.b.c[0] && a.b.c[0].d ......) {
        ...
    }
  • If using third-party libraries, is the app using deprecated methods? Use the latest APIs as recommended by the libraries.
  • Are all console logs and debuggers removed?
  • Are listeners removed before being re-added? Does the handler run multiple times on the same event on the same element? One way to check is by printing logs in the handler.
  • Pay attention to elements (usually inputs) that are listening to multiple events, such as keydown, keypress, and keyup. When one user action causes all events to fire, such that the handler is run once for each fired event, is the app still behaving as expected? Even if it is behaving correctly, does it make the app less responsive?
  • If the app architecture is based on the modular pattern, is one module referencing or modifying DOM elements of an unrelated module, thus breaking the pattern?

If the app uses jQuery

  • Are all jQuery object variables prefixed with $ for quick identification?
  • Are jQuery objects cached, if used more than once?
  • Are jQuery calls chained?
  • Are selectors cached in a string variable if used more than once? When no element is found for a selector and your code operates on this “missing” selector, jQuery fails silently, which is both GOOD and BAD at the same time. It is good because no error is thrown at run time; it is bad because the app may fail in other ways. A common reason is that the selector was changed and it was not updated globally. Cache the selector to minimize that risk.
  • Is the code using jQuery event delegation to minimize number of listeners?
  • How are you listening to events on elements that can be re-rendered? Via jQuery event delegation? That way, you don’t have to set up listeners again every time the element is re-rendered. It is easy to miss the re-add, especially when there are multiple render paths.
  • Are you using deprecated methods? For example, use .on() instead of .bind()/.live().

Browser support

One difficulty in front-end development is getting a web app to work well across devices, operating systems, and browsers. With the large number of device, operating system, and browser combinations in the wild, including the old and the native, working well everywhere is an impossible task for developers. The reasonable approach is to get your app’s traffic breakdown and test the most popular combinations during development.

To review layout and browser quirks, an in-person code review is the most effective as the reviewer can play with the feature/fix while reviewing the code. (A very simple exercise, which is also very effective in finding layout bugs, is resizing the browser window.) If an in-person review is not feasible, attach screenshots of the app to the PR, showing the layouts in different forms:

  • Desktop browser
  • Tablet browser (both portrait and landscape mode)
  • Phone browser (both portrait and landscape mode)

And finally

Code review is for more than the pull request at hand. Use it as a chance to refine the team’s standards and best practices and to discuss implementation strategies. While code review is an essential part of any development, it is especially important for front end as the front-end world changes very quickly. Team discussion via review comments is an effective way for all team members to get on the same page, learn from each other, and improve as a team.

How much time do you spend on code reviews? Do you scope for it in your sprint planning? Do you already look at some of the things mentioned above? What other specific things do you focus on?

Mobile First – A Retrospective

Responsive_Web_DesignMost engineers would agree that simply having a mobile-first mindset is not enough to build a high-quality responsive website — we also need relevant mobile-first guidelines and principles. We recently explored the challenges of scaling a mobile-oriented site to tablet and desktop. What we found was not always pretty, but the lessons learned were valuable. In this article, we will explore those lessons, in hopes that we can all improve how we build across mobile, tablet, and desktop.

Building a fully scalable website requires a strong focus on code quality. Concepts such as modularity, encapsulation, and testability become extremely important as you move across domains. Whether we are scaling up to desktop or down to mobile, we need the code to stay consistent and maintainable. Every hacked, poorly planned, or rushed piece of code we might add reduces our ability to write elegant, scalable, responsive code.

Perhaps creating a responsive app is not high on your team’s priority list right now. But one day it will be — and the conversion time frame might be very tight when that day comes.

Ideally, all you need to do is add media query CSS and everything just works. But the only way that can happen is if the code readily adapts to responsive changes.

Below are some suggestions and fixes that will make conversion to responsive easier. Some are specific to responsive design while others are general good practices.

Media queries

Yes, we all know about media queries. How hard can they be? Sprinkle some on any page and you have a responsive website, right?

Using media queries on your pages is essential; they allow you to overwrite CSS values based on screen size. This technique might sound simple, but in a larger project it can quickly get out of hand. A few major problems can get in the way of using media queries properly:

  • Colliding media queries: It is easy to make the mistake of writing media queries that overwrite each other if you do not stick to a common pattern. We recommend using the same boilerplate throughout all projects, and have created one here.
  • Setting element styles from JS: This is a tempting, but inferior, approach to building responsive websites. When an element relies on JS logic to set its width, it is unable to properly use media queries. If the JS logic is setting width as an inline property, the width cannot be overwritten in CSS without using !important. In addition, you have to now maintain an ever-growing set of JS logic.
  • Media queries not at the bottom: If your queries are not loaded last, they will not override their intended targets. Every module might have its own CSS file, and the overall ordering might not place it at the bottom, which leads us to our next point.
  • CSS namespacing for encapsulation: If you are writing a module, its CSS selectors should be properly encapsulated via namespace. We recommend prefixing class names with the module name, such as navbar-parent. Following this pattern will prevent conflicts with other modules, and will ensure that media queries at the bottom of your module’s CSS file override their intended targets.
  • Too many CSS selectors: CSS specificity rules require media queries to use the same specificity in order to override. It is easy to get carried away in LESS, which allows you to nest CSS multiple levels deep. While it can be useful to go one or two levels deep for encapsulation, usually this is unnecessarily complicating your code. We recommend favoring namespacing over nested specifiers as it is cleaner and easier to maintain.
  • Using !important to override styles: Adding !important to your styles reduces maintainability. It is better to avoid relying on !important overrides and instead use CSS namespacing to prevent sharing between modules.

Responsive or adaptive?

Both responsive and adaptive web design techniques contain powerful tools, but it is important to understand the differences between the two. Responsive techniques usually include media queries, fluid grids, and CSS percentage values. Adaptive techniques, on the other hand, are focused more on JavaScript logic, and the adding or removing of features based on device detection or screen size.

So, which should you use? Responsive or adaptive? The answer depends on the feature you are trying to implement. It can be tempting to jump straight into applying adaptive techniques to your feature, but in many cases it may not be required. Worse, applying adaptive techniques can quickly over-complicate your design. An example of this that we saw in many places is the use of JavaScript logic to set CSS style attributes.

Use JavaScript sparingly

When styling your UI, JavaScript should be avoided whenever possible. Dynamic sizing, for example, is better done through media queries. For most UI designs, you will be deciding on layouts based on screen size, not on device type. Confusing the need for device detection with screen size can lead us to apply adaptive where responsive would be superior.

Rethink any design that requires CSS attributes to change based on device detection; in almost all cases it will be better to rely on screen size alone, via media queries. So, when should we use adaptive Javascript techniques?

When to use adaptive

Adaptive web design techniques are powerful, as they allow for selective loading of resources based on user agent or screen size. Logic that checks for desktop browsers, for example, can load high-resolution images instead of their mobile-optimized counterparts. Loading additional resources and features for larger screens can also be useful. Desktop browsers, for example, could show more functionality due to the increased screen size, browser capability, or bandwidth.

Ideally, additional resources will be lazy-loaded for their intended platforms. Lazily loading modules helps with site speed for mobile web, while still allowing for a full set of functionality for desktop and tablet web. This technique can be applied by checking the user agent on the client or server. If done on the server, only resources supported by the user’s platform should be returned. Alternatively, client-based lazy loading can use Ajax requests to load additional resources if they are supported. This effect can be achieved using client-side JavaScript, based on browser support or user agent. Client-side detection is generally preferred, as it allows feature detection based on actual browser functionality instead of potentially complicated user agent checks.

Simple flex grid example

A responsive flex grid doesn’t have to be complicated. In our live demo page, we show a simple implementation that creates a horizontally scrolling section of image containers. The images are centered, allowed to expand up to 100% of their container, and will maintain their original aspect ratio. In addition, the container height values are set to 100%, allowing us to adjust the height in the parent wrapper only, and keeping our media query overrides simple and easy to read.

The html and css source code use the concepts mentioned above. We plan to add more boilerplate patterns; please don’t hesitate to add your own as well. Pull requests are welcomed!

Best practices

We hope that the information above will come in handy when you are working on your next mobile-first web project. Below is a summary of what we mentioned above and other helpful tips.

On using media queries

  • Most responsive layout can and should be done with media queries. JS manipulation of CSS (maybe with the exception of adding/removing classes) should be avoided. Setting width in JS is not as maintainable or dynamic compared to CSS.
  • Use media query boilerplate to ensure you do not have contradicting media queries or have media queries that are always skipped.
  • Put media queries at the bottom. Media queries override CSS and should be the final overrides, whether page level or module level.
  • If your regular CSS rules have many selectors, your media query CSS rules will have to as well, due to CSS specificity rules. Use as few selectors as possible when defining CSS rules.

 On writing CSS

  • Use CSS classes, not CSS IDs, to avoid CSS specificity issues.
  • Use the fewest number of selectors possible to define your selector.
  • Reuse classes. If an element has the same look on different parts of the page, do not create two different classes. Make a generic class and reuse it.
  • Encapsulate your CSS selectors by using proper namespacing to prevent conflicts.

e.g., class=”module-name-parent”

  • It is very rare that you need to use !important. Before you use it, ask yourself whether you can instead add another class (parent or same level). And then ask yourself whether the rule you are trying to override has unnecessary selectors.

 On writing LESS

  • Use LESS nesting only where needed. Nesting is good for organization, but it is also a recipe for CSS specificity issues.
  • Check that you do not have a CSS rule that looks like this:
    #wrapper #body-content #content #left-side #text {
        border: 1px solid #000;
    }
  • Work with the design team and define LESS variables using good names. Then, use these LESS variables everywhere possible.
  • If you are using a set of CSS rules repeatedly, make it a LESS mixin.

 On adding element wrappers

  • Most dom structures are more complex than necessary.
  • Add a wrapper only when needed. Do not add a wrapper when proper CSS can do the same thing.
  • If you remove the wrapper and the layout does not change, you do not need it. Now, do a global search for this wrapper’s references (JS, CSS, rhtml, jsp, tag) and remove them.

On lazy loading

  • Add a placeholder to your component for lazy loading.
  • Lazy-loaded sections will start off empty, so make sure you reserve the correct amount of space for this behavior. Otherwise, you will see the page shift as modules load in.
  • Use media queries for the empty section so that it closely matches the filled size.

On cleaning up

  • If you are playing around with CSS to attempt a layout and it starts working, remember to remove the unnecessary CSS rules. Many of them are probably not needed anymore. Remove the unnecessary wrappers as well.

Image source:  http://upload.wikimedia.org/wikipedia/commons/e/e2/Responsive_Web_Design.png