siroemk’s blog

フィヨルドブートキャンプで学んでいます(2020/11〜)

【CSS】メンターさんによるコードレビュー(受講生276人分)を全て読んでみた話

この記事はフィヨルドブートキャンプ Part 2 Advent Calendar 2020 - Adventarの13日目の記事です。

昨日は takopommさんの CSSで図形を描く復習 - takopomm’s diaryでした。

CSSでクリスマスツリーを作るなんて素敵ですね!

フィヨルドブートキャンプ のアドベントカレンダーは、どの記事も名作揃いなので ぜひぜひ読んでみてくださいね〜!

フィヨルドブートキャンプ のアドベントカレンダー Part 1Part 2

目次

はじめに(コードレビューを読んでみた理由)

f:id:siroemk:20201212233225j:plain

さてさて、ここからが本題です。

フィヨルドブートキャンプでは現役のエンジニアやデザイナーさんにコードレビューをしていただく課題があります。

コードレビューと聞くと少し緊張しますよね。

でも、自分の理解していなかったところが分かるし、一人で学習するよりも効率的だなと思っています。

フィヨルドブートキャンプでは自分のコードレビューが終わると、他の受講生が書いたコードやコードレビューも見ることができるようになります。

同じ見た目でもコードは全く違ったり、別の技術を使っていたりと、読んでみるだけでも面白いんですよ。

今回は

そのコードレビューを受講生全員分 読めば、なんだか力がつきそうだし、面白そうじゃん!?

ということで、歴代受講生がうけたコードレビューの全て読んでみることにしました。

総勢276人分!

(276人分読んだのは私とレビューをしてくれているメンターの町田さんだけでしょう!!!おおい!!!)

今回読んでみたコードレビューは?

今回読んでみたのは CSS上級 という課題のレビューです。

この課題は↓このようなページをCSSでコーディングしていくものになります。

HTMLには一切触らずにCSSでレイアウトを近づけていきます。

(全てのタグにクラス名が振られています)

f:id:siroemk:20201212231555p:plain

CSSの擬似要素でアイコンを作ったり、Flexboxを使ったり、フォームがあったり...となかなか手強い相手ですね。

私もこの課題に20時間くらい かかったような...

今回はこの課題のコードレビュー読んでみて「これはよく指摘されてる」「大事だな」と思ったところをまとめてみます。

CSSを独学で勉強している方や、これから課題に取り組むフィヨルドブートキャンプ生の役に立つかもしれません。

ちなみに、これからこの課題を取り組む方もいらっしゃるので、そのまま答えになってしまうようなコードは載せません。

こういうレビューが多かったよ!

1. そこにあるべき余白がない

.article {
  padding: 16px;
}

余白を指定するプロパティのpaddingmarginが指定されていないというレビューが多かったです。

文章や写真などの余白はもちろん、お問い合わせフォームのtextareapaddingが指定されていないなど、素人は見落としてしまいそうなものもありました。

余白がないと読みにくかったり、要素の分かれ目がわかりにくかったりするので細かいところまで実装しましょう!

見出しやタイトルの出だしなどのグリッドを揃えてあげると綺麗に見えます。

2. 行間がないと詰まっちゃう!

.article__paragraph {
  font-size: 16px;
  line-height: 1.8;
}

行間を指定するline-heightが指定されていないと行と行の間が詰まってしまい、読みにくくなってしまいます。

長い文章だと1.8、見出しだと1.3〜1.5くらいが読みやすい。

line-heightを指定するだけで素人っぽさがなくなりますね!

また、見出しやタイトルなども複数行になる可能性があるのでline-heightを指定しておきましょう。

3. スクロール(overflow)について

ul {
   overflow-y: auto;
}

f:id:siroemk:20201212234949p:plain

課題の左メニューにある、このリンクは縦スクロールで実装するようになっています。

縦スクロールはoverflow-yプロパティを使えば実装できますね。

レビューで多かったのは、overflow-y: auto;ではなく、overflow: scroll;を指定しているというものでした。

overflowプロパティだと縦にスクロールだけでなく、横スクロールも出来てしまいます。

今回の場合は縦スクロールだけのデザインなのでoverflow-yを使う方が適切です。

あと、overflow-y: scroll;ではなく、auto で設定すると、全てを表示しきれなかったときだけスクロールバーが表示されるようになるので、実際の仕事では auto を使う機会の方が多いそうです。

4. グローバルナビの文字しかクリックできない

<nav>
  <ul>
    <li><a href="#">ナビゲーション</a></li>
    <li><a href="#">ナビゲーション</a></li>
    <li><a href="#">ナビゲーション</a></li>
    <li><a href="#">ナビゲーション</a></li>
    <li><a href="#">ナビゲーション</a></li>
  </ul>
</nav>

一番多かったレビューは「グローバルナビのボタンではなく文字の上でしかクリックできない」というものでした。

f:id:siroemk:20201213141430p:plain

この赤いところしかクリックができないので操作性が悪いというレビューですね。

これはリンクであるaタグの初期値がdisplay: inline;であるからです。

inlineの要素にはheightwidthは指定できないため、幅と高さは「要素の中身」になってしまいます。

そのため、aタグの「グローバルナビ」という文字だけしかクリックできない状態になります。

aタグにdisplay: block;を指定してあげるとボックス全体をクリック出来るようになります。

もしくはflexboxを使ってaタグにdisplay: flex;を指定し、align-itemsプロパティとjustify-contentプロパティで位置を調整するというやり方もありました。

フレックスボックス - ウェブ開発を学ぶ | MDN

5. グローバルを汚染する!!?

img {
    margin: 20px;
}

a { 
    text-decoration: none;
}

全てのタグや全てのimgタグ、aタグなど、影響範囲の大きい設定についてのレビューもありました。

全てタグや imgタグやaタグ などにスタイルが適用すると今後新しい項目が出てきたときにメンテナンスしにくくなるかもよというレビューです。

例えば、a { text-decoration: none;} としてしまうと、text-decoration が必要な場合に、毎回、その設定を上書きしなくてはいけなくなります。

1つや2つなら気にならないかもしれないですが、量が増えてくると毎回上書きしなくてはならなくなり、コードが複雑になってしまいます。

現場では「グローバルを汚染する」なんて言うそう。

もちろん、デザインによってはimgタグやaタグに指定した方がコードが短くなる場合もあるので間違いではないですが、後先考えずに指定にするのは避けた方が無難です。

6. 非表示にしたCSSが残ってます...

.global-nav__link {
/*   background: orange; */
/*   background: tomato; */
  background: red;
}

CSSを書いてる時にコメントアウトしたコードは必要なければ必ず削除するようにしましょう。

一緒に作業する仲間や未来の自分が「このコメントアウトには何か意図がある?」と考えてしまいます。

いらないコードは消してしまった方が親切ですね。

7. 何度も出てくる値には変数を使う

/*変数を宣言*/
:root {
  --line-height-val: 1.8;   /*行高*/
}  

.article__paragraph {
  line-height: var(--line-height-val);    
}

.article__blockquote {
  line-height: var(--line-height-val);    
}

何度も使われている値は変数でまとめておくと、値の変更があったときに変数を修正するだけで全てのコードに反映できるので便利です。

その値が何を意味しているのかも分かりやすくなりますね。

変数は--から始めて、好きな変数名をつけます。

定義した変数を呼び出すにはvar(--変数名)と記述します。

CSS カスタムプロパティ (変数) の使用 - CSS: カスケーディングスタイルシート | MDN

良いCSSとは

f:id:siroemk:20201213112402j:plain

コードレビューを読んでみて

良いCSSとは「これから起きうる変化に対応しやすいコード」ではないのかと思いました。

  • 今後見出しや文章の量が増えるかもしれない
  • HTMLの構造を変えることがあるかもしれない
  • 新しい要素を差し込むかもしれない
  • 全体のフォントサイズを変えることがあるかもしれない
  • 新しい人がチームに入ってくるかもしれない
  • サイトのコンセプトが変わってくるかもしれない

将来をイメージしながら、こういう変化があるかもしれないと念頭に置くと良いCSSが書けるかもしれません。

さいごに

コードレビューを読む前までは「CSSってシンプルだしそんなに難しくないんじゃない?」とと思っていました。

でも、シンプルだからこそ自由度が高く、たくさんの人と作り上げていく時は各自の個性が出てくるため、CSSは破綻しやすいということが分かりました。

調べれば調べるほどCSSって奥深いのでは...沼では...という気持ちになります。

舐めていましたCSS...

CSSの設計についても本を読みたいです。

良い本があれば紹介してください...

また、コードレビューにはあまり関係ないですが、「どんどん質問している方ほど完成品のクオリティがあがっている傾向があるのでは」と感じました。

私もどんどん質問していこうと思うきっかけにもなり、良かったです。

ありがとう、コードレビュー。