diff --git a/package-lock.json b/package-lock.json index 3253ef1..539696b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,8 @@ "is-seo-everything", "hooks-testing", "frontend-conf-hokkaido", - "ts-overload-function" + "ts-overload-function", + "review-comment" ], "dependencies": { "@iconify-json/mdi": "^1.2.0", @@ -2060,6 +2061,10 @@ "resolved": "reuse", "link": true }, + "node_modules/@slide/review-comment": { + "resolved": "review-comment", + "link": true + }, "node_modules/@slide/the-tooltip": { "resolved": "the-tooltip", "link": true @@ -13309,6 +13314,12 @@ "license": "ISC", "devDependencies": {} }, + "review-comment": { + "name": "@slide/review-comment", + "version": "1.0.0", + "license": "MIT", + "devDependencies": {} + }, "the-tooltip": { "name": "@slide/the-tooltip", "version": "1.0.0", diff --git a/package.json b/package.json index 3c39b5d..841d01f 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/reuse/me.md b/reuse/me.md index f614ce6..c55ea65 100644 --- a/reuse/me.md +++ b/reuse/me.md @@ -13,29 +13,34 @@ layout: cover
Frontend developer
- +
- 技術記事を書いたりするのが趣味。
+ ブラウザの標準化まわりを追うのが趣味
最近はReactを使ったアプリケーションを書いています。
ユーザーインターフェイスやブラウザが好き。
レビュープロセスを見直す機会があったのでその紹介
+ +--- + +## レビューコメントのラベル + +レビューコメントには必ずラベルを付けてもらう運用にしていた。 + +| ラベル | 意味 | +| -------- | -------------------------------------------- | +| Must | 必ず直してほしい箇所 | +| IMO | 任意だが自分ならこうする的な意見 | +| NITS | 直さなくても問題ないが修正したほうが良い箇所 | +| Question | 質問 | + +- 導入当初は分かりやすくていい印象 +- チームでコミュニケーションをしていると改善点が見つかる + +--- + +## レビューコメントのラベル + +導入からしばらく経っていたのでちょっと整理してみることに + +問題点はなんとなく候補があったので、ドキュメントを書いてチームに共有してみた。 + +--- + +## 問題点 + +- IMOとNITSの使い方が人によって異なる +- Questionが多用され着地点が分からないコメントがある + + + + + +--- +layout: center +--- + +## 改善しよう + +--- + +### IMOとNITSの変更要望度が人によって異なる + +話し合うと、ラベルは変更してほしい度合いで使い分けていたことが分かった。 + +なのでざっくり下記のような対応を行うことに。 + +#### IMOとNITSの変更要望度が人によって異なる + +- ラベルの使い分けを変更要望度を軸としたものに +- レイヤーが分かれるようにラベルを定義する + +#### Questionが多用され着地点が分からないコメントがある + +- 質問っぽい指摘を無くす +- 質問としてコメントされた場合はコードの変更はしない + +--- + +### IMOとNITSの変更要望度が人によって異なる + + + + + +変更要望度に合わせてラベルを整理してみる。 + +--- + +### IMOとNITSの変更要望度が人によって異なる + +ある程度整理できたので、変更要望度を軸にドキュメントに記載。 + +| ラベル | 変更要望度 | 利用シーン | +| ---------- | ---------- | ------------------------------------------------ | +| Must | 100% | 明らかなバグ・仕様と実装の乖離・ガイドライン違反 | +| Want | 99%-50% | パフォーマンスや可読性など非機能要件的な指摘 | +| IMO / NITS | 49%-1% | (主観的な)より良い書き方の提案 | +| Ask | 0% | 質問 | + +どういった場面で利用するのかなどもある程度書いておく。 + +--- +layout: center +--- + +## Questionが多用され着地点が分からないコメントがある + +--- + +## Questionが多用され着地点が分からないコメントがある + +純粋な質問まで禁止してしまうのは厳しいのでラベルは残すことになった。 + +その上で指摘は指摘として他のラベルを使ってもらうように。 + +- 難しそうな実装部分はモブレビューを実施してそのログを残す +- 「大丈夫そうですか?」みたいな変更を期待した質問はNGに +- レビューコメントの書き方を工夫して「質問っぽい指摘」になるのを避ける + + + +--- + +### 質問っぽい指摘を避けるための書き方 + +質問を避けるために文章を工夫する方法などもドキュメントにまとめてみた。 + +書いたこととしては下記の3つの順番でコメントを書くといいよ、という内容でした。 + +- 前提として自分の認識 +- 前提が正しかった場合にどのような懸念があるか +- それを解決する方法 + +これによって曖昧な部分も自分の認識を前提として変更要望度が出せるように。 + +--- + +## まとめ + +- レビューのラベルは変更要望度を基準に整理するといい +- 質問っぽい指摘は避けてなるべく具体的にコメントを書く diff --git a/review-comment/styles/index.ts b/review-comment/styles/index.ts new file mode 100644 index 0000000..f1c9f2c --- /dev/null +++ b/review-comment/styles/index.ts @@ -0,0 +1,2 @@ +import "./mod.css" +import "@slide/reuse/styles"; \ No newline at end of file diff --git a/review-comment/styles/mod.css b/review-comment/styles/mod.css new file mode 100644 index 0000000..c12a809 --- /dev/null +++ b/review-comment/styles/mod.css @@ -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); + } +}