Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

レビューを少し見直した話 #295

Merged
merged 10 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"is-seo-everything",
"hooks-testing",
"frontend-conf-hokkaido",
"ts-overload-function"
"ts-overload-function",
"review-comment"
],
"devDependencies": {
"@chromatic-com/storybook": "^2.0.2",
Expand Down
15 changes: 10 additions & 5 deletions reuse/me.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,34 @@ layout: cover
<h1 style="margin: 0 0 0 -3px; line-height: 1;">ken7253</h1>
<p style="margin: 0;">Frontend developer</p>
</div>
<img alt="" src="https://dairoku-studio.com/ogp-thumbnail.png" style="position:absolute; top:0; right:0; width: 200px; border-radius: 100vh; mix-blend-mode: overlay;">
<img alt="" src="https://dairoku-studio.com/ogp-thumbnail.png" style="position:absolute; top:0; right:50px; width: 200px; border-radius: 100vh; mix-blend-mode: overlay;">
</div>

<div>
<p style="font-size: 1rem; line-height: 2.1;">
技術記事を書いたりするのが趣味。<br>
ブラウザの標準化まわりを追うのが趣味<br>
最近はReactを使ったアプリケーションを書いています。<br>
ユーザーインターフェイスやブラウザが好き。
</p>
</div>

<div style="display: flex; gap: 12px; flex-direction: column;">
<a href="https://github.com/ken7253" style="border: none; display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<a href="https://github.com/ken7253" style="display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<span style="display: flex; gap: 8px; align-items: center; line-height: 1; font-size: 0.75rem;">
<radix-icons-github-logo />https://github.com/ken7253
</span>
</a>
<a href="https://zenn.dev/ken7253" style="border: none; display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<a href="https://zenn.dev/ken7253" style="display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<span style="display: flex; gap: 8px; align-items: center; line-height: 1; font-size: 0.75rem;">
<simple-icons-zenn/>https://zenn.dev/ken7253
</span>
</a>
<a href="https://dairoku-studio.com/" style="border: none; display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<a href="https://bsky.app/profile/ken7253.bsky.social" style="display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<span style="display: flex; gap: 8px; align-items: center; line-height: 1; font-size: 0.75rem;">
<simple-icons-bluesky/>https://bsky.app/profile/ken7253.bsky.social
</span>
</a>
<a href="https://dairoku-studio.com/" style="display: flex; flex-direction: column; gap: 8px; width: fit-content;">
<span style="display: flex; gap: 8px; align-items: center; line-height: 1; font-size: 0.75rem;">
<mdi-web style="scale: 1.2;"/>https://dairoku-studio.com
</span>
Expand Down
14 changes: 14 additions & 0 deletions review-comment/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"name": "@slide/review-comment",
"version": "1.0.0",
"main": "index.js",
"scripts": {
"dev": "slidev",
"build": "slidev build",
"export": "slidev export"
},
"keywords": [],
"author": "ken7253 <[email protected]>",
"license": "MIT",
"description": ""
}
Binary file added review-comment/public/image/label.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review-comment/public/image/question.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review-comment/public/image/review.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review-comment/slides-export.pdf
Binary file not shown.
158 changes: 158 additions & 0 deletions review-comment/slides.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
---
theme: default
titleTemplate: '%s - ken7253'
colorSchema: 'dark'
fonts:
sans: 'M PLUS 2'
mono: 'M PLUS 1 Code'
---

# レビューのやり方を(ちょっと)整理した話
@[IVRyエンジニア忘年LT大会2024](https://connpass.com/event/333537/)

---
src: "../reuse/me.md"
---

---
layout: section
---

## みなさん!レビューしてますか?

---
layout: section
---

## レビューは大変

---
layout: center
---

<p style="font-size: 2rem;">レビュープロセスを見直す機会があったのでその紹介</p>

---

## レビューコメントのラベル

レビューコメントには必ずラベルを付けてもらう運用にしていた。

| ラベル | 意味 |
| -------- | -------------------------------------------- |
| Must | 必ず直してほしい箇所 |
| IMO | 任意だが自分ならこうする的な意見 |
| NITS | 直さなくても問題ないが修正したほうが良い箇所 |
| Question | 質問 |

- 導入当初は分かりやすくていい印象
- チームでコミュニケーションをしていると改善点が見つかる

---

## レビューコメントのラベル

導入からしばらく経っていたのでちょっと整理してみることに

問題点はなんとなく候補があったので、ドキュメントを書いてチームに共有してみた。

---

## 問題点

- IMOとNITSの使い方が人によって異なる
- Questionが多用され着地点が分からないコメントがある

<img src="/image/review.png" width="400" style="margin: 64px 0 0 auto; mix-blend-mode: plus-lighter;" alt="人によって利用するラベルが違うことを表した図">

<!-- https://excalidraw.com/#json=3cnobLM0GllkyPjPMpGhp,vWhkxvfQSR1z7fckyMVVaA 質問なのに空気を読んで修正する的なことがちょっとあった -->

---
layout: center
---

## 改善しよう

---

### IMOとNITSの変更要望度が人によって異なる

話し合うと、ラベルは変更してほしい度合いで使い分けていたことが分かった。

なのでざっくり下記のような対応を行うことに。

#### IMOとNITSの変更要望度が人によって異なる

- ラベルの使い分けを変更要望度を軸としたものに
- レイヤーが分かれるようにラベルを定義する

#### Questionが多用され着地点が分からないコメントがある

- 質問っぽい指摘を無くす
- 質問としてコメントされた場合はコードの変更はしない

---

### IMOとNITSの変更要望度が人によって異なる

<img src="/image/label.png" style="background-color: rgb(51 51 51 / 70%);padding: 32px;margin-top: 16px;border-radius: 16px;" alt="ラベルの整理を表した図、Must/IMO/NITS/Questionがバラバラから、Must/Want/IMO or NITS/Askという順になっている">

<!-- https://excalidraw.com/#json=JdU3mJdujM7nVJ8mr9tjw,lWGc1uSH_X9UrSXUQH6pjQ -->

変更要望度に合わせてラベルを整理してみる。

---

### IMOとNITSの変更要望度が人によって異なる

ある程度整理できたので、変更要望度を軸にドキュメントに記載。

| ラベル | 変更要望度 | 利用シーン |
| ---------- | ---------- | ------------------------------------------------ |
| Must | 100% | 明らかなバグ・仕様と実装の乖離・ガイドライン違反 |
| Want | 99%-50% | パフォーマンスや可読性など非機能要件的な指摘 |
| IMO / NITS | 49%-1% | (主観的な)より良い書き方の提案 |
| Ask | 0% | 質問 |

どういった場面で利用するのかなどもある程度書いておく。

---
layout: center
---

## Questionが多用され着地点が分からないコメントがある

---

## Questionが多用され着地点が分からないコメントがある

純粋な質問まで禁止してしまうのは厳しいのでラベルは残すことになった。

その上で指摘は指摘として他のラベルを使ってもらうように。

- 難しそうな実装部分はモブレビューを実施してそのログを残す
- 「大丈夫そうですか?」みたいな変更を期待した質問はNGに
- レビューコメントの書き方を工夫して「質問っぽい指摘」になるのを避ける

<img src="/image/question.png" alt="" width="400" style="margin: 0 0 0 auto">

---

### 質問っぽい指摘を避けるための書き方

質問を避けるために文章を工夫する方法などもドキュメントにまとめてみた。

書いたこととしては下記の3つの順番でコメントを書くといいよ、という内容でした。

- 前提として自分の認識
- 前提が正しかった場合にどのような懸念があるか
- それを解決する方法

これによって曖昧な部分も自分の認識を前提として変更要望度が出せるように。

---

## まとめ

- レビューのラベルは変更要望度を基準に整理するといい
- 質問っぽい指摘は避けてなるべく具体的にコメントを書く
2 changes: 2 additions & 0 deletions review-comment/styles/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import "./mod.css"
import "@slide/reuse/styles";
39 changes: 39 additions & 0 deletions review-comment/styles/mod.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#app h1 {
font-size: 2.5rem;
}

div#app h3 {
position: relative;
z-index: 10;

&::after {
content: "";
position: absolute;
bottom: 0;
left: 0;
width: 100%;
height: 60%;
transform: skewY(0.5deg) translateY(3px);
background-color: var(--c-accent);
z-index: -1;
opacity: 0.7;
}
}

div#app h4 {
position: relative;
margin-top: 1.5rem;
z-index: 10;

&::after {
content: "";
position: absolute;
bottom: 0;
left: -6px;
width: 100%;
height: 60%;
z-index: -1;
background-color: var(--c-secondary);
filter: blur(2px);
}
}
Loading