LoginSignup
5
1

More than 1 year has passed since last update.

@xrxoxcxox(Keisuke Watanuki)inQiita株式会社

CSSのコードレビューのときに見ていること

Last updated at Posted at 2019-12-19

社内のレビューで気をつけていることを書き出してみたらかなり一般的な内容だったので、せっかくだしQiitaに投稿します。

なお👀のマークをつけている項目は「環境が整っていなければ確認しているけどフォーマッターに任せたい」項目です。

それでは早速行きましょう！

👀誤字がないこと・存在しないプロパティや値を指定していないこと

  • プロパティや値の誤字はフォーマッターでチェックします
  • class名の誤字はフォーマッターでは拾えないと思うのでスペルチェッカーを入れておきます
NG例
/* 存在しないプロパティを指定している */
.foo {
  wide: 100px;
}

/* 綴りが間違っている */
.informetion {
  color: red;
}
OK例
/* プロパティが存在している */
.foo {
  width: 100px;
}

/* 綴りが正しい */
.information {
  color: red;
}

👀スペース、改行、インデントがルール通りに統一されていること

  • ※ルールがあることが前提です。
    • ない場合は作るか、stylelint-config-standard等のスタンダードっぽい設定に乗っかります。
    • OK例は、このルールが最高！と押し付けるつもりはなく、全体を通して統一されていることが大事です。
  • 動作に影響はないですが、見辛いので統一しましょう。
NG例
/* {}の前後のスペースや改行が揃っていない */
/* :や;の前後のスペースの有無が揃っていない */
/* インデントが揃っていない */
.foo {
  width:100px;
}
.bar {
color: red ;
}

.baz {
  display : flex ;
}
OK例
/* スペース、改行、インデントが揃っている */
.foo {
  width: 100px;
}

.bar {
  color: red;
}

.baz {
  display: flex ;
}

👀カラーコードの書き方が統一されていること

  • ルールがあること前提です。
  • どの書き方が良いというつもりはありません。チーム内で決めたルールに沿うことが大事です。
NG例
/* 省略するしないが混在している */
/* 大文字小文字が混在している */
.foo {
  color: #FF0000;
}

.bar {
  color: #0f0;
}
OK例
/* 統一されている */
.foo {
  color: #f00;
}

.bar {
  color: #0f0;
}

👀重複したセレクタやプロパティがないこと

  • 重複している＝打ち消しが発生しているので、ただただBADです
NG例
/* .fooが重複している */
/* .barの中のcolorが重複している */
.foo {
  width: 100px;
}

.bar {
  color: red;
  font-size: 14px;
  color: blue;
}

.foo {
  width: 200px;
}
OK例
/* 重複ナシ */
.foo {
  width: 200px;
}

.bar {
  color: blue;
  font-size: 14px;
}

class名やid名がルールに則って命名されていること

  • こちらもルール自体は何でも良いのですが、ルールに則っていることが大事です。
    • スネークケースが良いとかキャメルケースが良いとかBEMが良いとか、そういうのはありません。
    • オレオレなルールであってもちゃんと統一されていれば（コードレビューの観点としては）問題ありません。
  • 意外とフォーマッターで解決できなかったので目視しています。
    • 出来るやり方をしっている人がいたら教えてください🙇‍♂️
NG例
/* MindBEMdingとローワーキャメルケースが混在している */
.foo__bar {
  width: 100px;
}

.fooBaz {
  color: red;
}
OK例
/* MindBEMdingで統一されている */
.foo__bar {
  width: 100px;
}

.foo__baz {
  color: red;
}

👀詳細度が高すぎる指定をしていないこと

  • 基本全部classセレクタで、入れ子もなく指定されているのが分かりやすくて個人的には好きです。
  • とは言え詳細度を上げることが必要な場面もありますので「詳細度が高すぎる指定をしない」という表現にとどめています。
NG例
/* .barが.fooの外に出たら適用されなくなる */
/* .barを上書きしたいときに大変 */
div .foo .bar {
  width: 100px;
}
OK例
/* スタイリングしたいclassそのものだけを記述している */
.bar {
  width: 100px;
}

プロパティの初期値を書いていないこと

  • これはもしかしたらプロパティによっては明示した方が良いシーンもあるかもしれません。
  • ですが、基本スタンスとしては最初から設定されているものをもう一度書くのは勿体無いと思っています。
NG例
/* 要素セレクタで指定しているのはわかりやすさのためです */
/* pのdisplayの初期値はblock */
p {
  display: block;
}

/* spanのdisplayの初期値はinline */
span {
  display: inline;
}

不要な値が書いていないこと

  • 苦労した後がそのまま残っていて、記述が肥大化しているコードを見かけることがあります。
NG例
/* 中央揃えにしたいらしい */
.foo {
  text-align: center;
  position: absolute;
  left: 0;
  right: 0;
  margin: auto;
}
OK例
/* 中央揃えになっている */
.foo {
  margin: auto;
}

変数が存在するときにベタで書いていないこと

  • SASSの変数、あるいはCSS変数などを使っている場合のみの話です
NG例
/* 定義済みの値を手で書いている */

/* variables.scss */
$main-color: #f00;

/* styles.scss */
.foo {
  color: #f00;
}
OK例
/* 定義済みの値を手で書いている */

/* variables.scss */
$main-color: #f00;

/* styles.scss */
.foo {
  color: $main-color;
}

マジックナンバーを使っていないこと

  • あとから見返したときに謎が謎を呼ぶので使わない。
  • 何かの計算結果の値であるならその計算式をコメントに書く。
NG例
/* CSSだけで作るアコーディオン、みたいなときに使われがちなコード（若干例が悪い気もする） */

.foo {
  max-height: 0;
  overflow: hidden;
}

/* この200pxはどこから出てきたのか分からない */
.bar:checked + .foo {
  max-height: 200px;
}
OK例
/* 理由がコメントで書いてある */
.foo {
  max-height: 0;
  /* アコーディオンが閉じているので高さが0になるように指定 */
  overflow: hidden;
}

.bar:checked + .foo {
  max-height: 200px;
  /* アコーディオンが開いたとき、中に高さ50pxのメニューx4つ存在しているので高さが200px */
}

👀適切なベンダープレフィックスを付与していること

  • 「適切な」というのは、過不足のないベンダープレフィックスを付与するということです。
NG例
/* IE11で崩れる */
.foo {
  display: grid;
  grid-template-columns: 100px 1fr 100px;
}

/* 存在しないベンダープレフィックスを書いている */
.bar {
  -ms-animation: someAnimation 300ms ease;
  -webkit-animation: fadeIn 1s ease;
  animation: someAnimation 300ms ease;
}
OK例
/* IE11でも大丈夫 */
.foo {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 100px 1fr 100px
  grid-template-columns: 100px 1fr 100px;
}

/* 必要なベンダープレフィックスだけを書いている */
.bar {
  -webkit-animation: fadeIn 1s ease;
  animation: someAnimation 300ms ease;
}

打ち消して実装せず、追加して実装していること

  • 例はちょっと極端ですが、普段だと@extendを使うときなどにも見ていることです。
  • CSSの性質上、打ち消しをたくさん発生させるよりは後から必要なものを付け加えた方が後々楽なことが多いです。
NG例
/* 段落ごとに上に10pxの余白をつけるつもりで指定 */
p {
  margin-top: 10px;
}

/* 余白の要らない段落には、全てに打ち消す記述をつけるハメに */
.paragraph {
  margin-top: 0;
  font-size: 14px;
}

.headline {
  margin-top: 0;
  font-size: 20px;
  color: #f00;
}
OK例
/* 全体にかかるmargin-topを消したことで余計な記述がなくなる */
.paragraph {
  font-size: 14px;
}

.headline {
  font-size: 20px;
  color: #f00;
}

presentational componentっぽい単位のクラスに余白を設定していないこと

  • 急にちょっとややこしくなりましたが、平たく言えば「ボタンやカードなど、単体の要素に余白を記述していないか」を見ています。
  • 上の話と少し似ていますが、繰り返し使われる単位に余白を設定してしまうとロクなことにならないので注意しています。
NG例
/* .buttonというclassには毎回margin-topがついてしまう */
.button {
  background-color: #f00;
  color: #fff;
  padding: 10px;
  margin-top: 20px;
}
OK例
/* .button自体へのmargin指定はしない */
.button {
  background-color: #f00;
  color: #fff;
  padding: 10px;
}

/* ユーティリティ系のclassを用意しておくか、componentとは別な単位のclassを作るかしてマルチクラスでmarginを設定するなど */

まとめ

  • 見ている箇所を挙げましたが、もっと色々あるような気がするので適宜追加していきます。
  • コードをレビューする人だけでなく書く人にもお役に立てたら嬉しいです。
  • フォーマッターは是非とも入れましょう👀
5
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
5
1