Conversation
📝 WalkthroughWalkthroughEventモデルに Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant JS as BookmarkJS
participant Rails as RailsApp
participant DB as Database
Browser->>JS: ユーザーが「Bookmark」ボタンをクリック
JS->>Rails: POST /bookmarks (bookmarkable_type=Event, id=...)
Rails->>DB: INSERT/DELETE bookmarks
DB-->>Rails: DB 操作成功
Rails-->>JS: JSON { status, bookmarked }
JS-->>Browser: ボタン表示を "Bookmark中" / aria 更新
Browser->>Rails: GET /current_user/bookmarks（ブックマークリスト表示）
Rails->>DB: SELECT bookmarks for user
DB-->>Rails: bookmarks list
Rails-->>Browser: ブックマークリストを返す
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
2 times, most recently
from
7c12934 to
0929f54
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/assets/stylesheets/application.css`:
- Around line 5-9: The CSS contains Tailwind v4 directives (`@source`, `@theme`,
`@import` "tailwindcss") which Biome's CSS linter doesn't natively support; update
biome.json to add the stylesheet directory to the "ignores" array (so Biome
skips these files) or disable the CSS linter in Biome config; target the file
patterns that match the CSS files containing `@source/`@theme directives when
editing the "ignores" list.
🧹 Nitpick comments (1)
app/assets/stylesheets/application/blocks/page-content/_page-content-header-actions.css (1)
16-64: 重複セレクタを統合して可読性を上げたい。同一セレクタの定義が複数箇所に散らばっており、将来の修正で差分がズレやすい状態です。1箇所にまとめるのを推奨します。
♻️ まとめる場合の一例
-@media (min-width: 40em) { - .page-content-header-actions { - justify-content: space-between; - gap: .5rem; - } -} - -.page-content-header-actions { - padding-top: .25rem; - display: flex; - flex-wrap: wrap; -} - -.page-content-header-actions__start { - justify-content: flex-start; -} - -.page-content-header-actions__start, -.page-content-header-actions__end { - display: flex; -} - -@media (min-width: 40em) { - .page-content-header-actions__start, .page-content-header-actions__end { - flex: 1 1; - gap: .5rem; - } -} +/* 既存ブロックへ統合 */ +.page-content-header-actions { + padding-top: .25rem; + display: flex; + flex-wrap: wrap; +} +@media (min-width: 40em) { + .page-content-header-actions { + justify-content: space-between; + gap: .5rem; + } +} +@media (max-width: 35.9375em) { + .page-content-header-actions { + justify-content: center; + gap: .25rem; + } +} + +.page-content-header-actions__start, +.page-content-header-actions__end { + display: flex; +} +.page-content-header-actions__start { + justify-content: flex-start; +} +.page-content-header-actions__end { + justify-content: flex-end; +} +@media (min-width: 40em) { + .page-content-header-actions__start, + .page-content-header-actions__end { + flex: 1 1; + gap: .5rem; + } +} +@media (max-width: 35.9375em) { + .page-content-header-actions__end { + gap: .25rem; + } +}
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
0929f54 to
805c7ed
Compare
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
bcba813 to
c1fbcaf
Compare
|
@djkazunoko |
|
@kutimiti1234 以前私も似たようなIssue・PRの対応（定期イベントにブックマークボタンを追加 by tyrrell-IH · Pull Request #9306 · fjordllc/bootcamp）をしましたが、ブックマークのラベル内の文字の折り返し対応について確認したいです。 私の実装では↓ではなく ↓になるように対応しました。 できれば同じような実装になるように調整したいので、現状どんな方針なのか教えていただけたら幸いです〜 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/components/BookmarksInDashboard.jsx (1)
123-132:Event型に「定期イベント」ラベルが表示される点は実装上の問題です。API レスポンスには各モデル型に対する正しいi18n翻訳（
modelNameI18n）が含まれており、eventは「特別イベント」、regular_eventは「定期イベント」と翻訳されます。現在のコードは両方に同じ「定期イベント」ラベルをハードコーディングしているため、Event型に対して誤った表示となっています。修正方法として、
RegularEventのみ特別な表示を行い、EventはmodelNameI18nで正しい「特別イベント」ラベルが表示されるようにしてください。現在のコード
{bookmark.modelName === 'RegularEvent' || bookmark.modelName === 'Event' ? ( <> 定期 <br /> イベント </> ) : ( bookmark.modelNameI18n )}
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
3 times, most recently
from
d85c5dd to
db51f40
Compare
@tyrrell-IH |
|
@kutimiti1234 |
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
db51f40 to
a59c52d
Compare
|
@kutimiti1234 |
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
a59c52d to
c41a009
Compare
djkazunoko
left a comment
djkazunoko
left a comment
There was a problem hiding this comment.
@kutimiti1234
動作確認問題ありませんでした！
2点コメントしましたのでご確認お願いします！
There was a problem hiding this comment.
#9306 で作成されたこちらのテストですが、日報だけではなく、ブックマークできるその他の全てのモデルの表示も確認したほうがいいように思いました。
There was a problem hiding this comment.
こちらのテストについてですが、そもそもが何のためのテストかというと、app\helpers\label_helper.rbに関するテストかと思います。そのため、特別イベント、定期イベント、その他の表示を加工する必要のないbookmarkの3種類のテストで十分だと考えています。再度検討いただければと思います。
There was a problem hiding this comment.
こちらのテストの網羅性について@komagataさんに質問してみました！
自分としては当初、「ブックマークできるその他の全てのモデルの表示も確認したほうが安全では？」と考えていました。今回確認した「日報」がたまたま改行されなかっただけで、他のモデルが想定外の表示になる可能性もゼロではない、テストするまでは確定しないと思ったためです。対象数も多くないので、全部書いてもよいのではと感じていました。
komagataさんの回答は「代表例だけでもよいし(現状のまま)、全部書くのもよい。どちらでもいい（実装者判断でOK）。」というものでした。
この処理自体は重要度がそこまで高くないため「特別イベント」「定期イベント」「その他代表例」の3パターンが確認できていれば十分と考えることもできるし、処理が重いテストでもなく対象が数個なら全部書いてもいいとのことでした。
この説明を受けて、自分としても「全モデル網羅までは必須ではない」と思ったため、現状のテスト内容のままで問題ないと判断しました。ですので修正は不要です👍
app/views/events/_event.html.slim
Outdated
| .page-content-header-actions__action | ||
| button(type="button" id="bookmark-button" data-bookmarkable-id="#{event.id}" data-bookmarkable-type="Event" class="a-bookmark-button a-button is-sm is-block is-inactive is-muted" disabled aria-pressed="false" aria-busy="true") | ||
| | Bookmark |
There was a problem hiding this comment.
ブックマークボタンは各ビューで使い回されているので、以下のようにパーシャルに分割するのがいいと思いました。
.page-content-header-actions__action
= render 'bookmarks/button', bookmarkable: event
There was a problem hiding this comment.
ありがとうございます。
こちらその通りだと考えたので共通化を施しました。
|
@kutimiti1234
(将来的にこのファイルは削除されると思いますが😅 #9045 ) |
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
3 times, most recently
from
cf94fe3 to
9e6eeed
Compare
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
9e6eeed to
14f52dd
Compare
|
@djkazunoko |
|
@kutimiti1234 さんの実装ではないですが、 |
|
@kutimiti1234
こちらで別途Issue立てるでも全然大丈夫ですので、必要であればおっしゃってください〜🙏 |
|
@tyrrell-IH @kutimiti1234 このPRで修正していいと思います！ |
|
@djkazunoko |
kutimiti1234
force-pushed
the
feature/make-event-bookmarkable
branch
from
2c32cc7 to
08deb91
Compare
|
@tyrrell-IH @djkazunoko @djkazunoko |
djkazunoko
left a comment
djkazunoko
left a comment
There was a problem hiding this comment.
対応ありがとうございます！
Approveさせていただきました〜
|
@djkazunoko @okuramasafumi |
4 participants
Issue
概要
特別イベントにブックマーク機能を追加した
変更確認方法
/events/994018171へ移動/current_user/bookmarksへ移動/events/994018171へ移動し、bookmarkを解除/current_user/bookmarksへ移動し、一覧から消えていることを確認。Screenshot
変更前
変更後
Summary by CodeRabbit
新機能
UI
テスト
✏️ Tip: You can customize this high-level summary in your review settings.