feat(image-viewer): 同メッセージ内の複数画像を連続して閲覧できるようにする#5093
Open
uni-kakurenbo wants to merge 2 commits intomasterfrom
Open
feat(image-viewer): 同メッセージ内の複数画像を連続して閲覧できるようにする#5093uni-kakurenbo wants to merge 2 commits intomasterfrom
uni-kakurenbo wants to merge 2 commits intomasterfrom
Conversation
|
Preview (prod) → https://5093-prod.traq-preview.trapti.tech/ |
📝 WalkthroughWalkthroughFileModalImage.vueコンポーネントに複数画像ナビゲーション機能を追加。兄弟画像リストを計算して左右ボタンを表示し、矢印キー入力に対応。トランジション効果とプリロード機能を実装。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5093 +/- ##
=======================================
Coverage 62.60% 62.60%
=======================================
Files 108 108
Lines 3097 3097
Branches 630 630
=======================================
Hits 1939 1939
Misses 1048 1048
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
src/components/Modal/FileModal/FileModalImage.vue (2)
122-136: 編集可能要素の除外条件はもう少し広げたいです。
input/textareaだけだと、contenteditableやselect系の要素上でもArrowLeft/ArrowRightがそのまま画像遷移になります。HTMLElement.isContentEditableとHTMLSelectElementも見ておくと事故りにくいです。🔧 修正案
const onKeyDown = (e: KeyboardEvent) => { - if ( - e.target instanceof HTMLInputElement || - e.target instanceof HTMLTextAreaElement - ) { + const target = e.target instanceof HTMLElement ? e.target : null + if ( + target instanceof HTMLInputElement || + target instanceof HTMLTextAreaElement || + target instanceof HTMLSelectElement || + target?.isContentEditable + ) { return } if (e.key === 'ArrowRight') { nextFile() } else if (e.key === 'ArrowLeft') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modal/FileModal/FileModalImage.vue` around lines 122 - 136, onKeyDown currently only ignores inputs and textareas, causing ArrowLeft/ArrowRight to trigger image navigation even when focus is on contenteditable or select elements; update the onKeyDown handler to first assert e.target is an HTMLElement and skip handling when e.target.isContentEditable is true or when e.target is an instance of HTMLSelectElement (in addition to existing checks for HTMLInputElement and HTMLTextAreaElement) so that nextFile()/prevFile() are not called while editing or selecting; keep useEventListener('keydown', onKeyDown) as-is.
89-97: プリロード対象は前後の画像だけに絞った方がよさそうです。ここだと sibling 全件を毎回
new Image()しているので、画像枚数の多い投稿を開いた瞬間に/files/:idへのリクエストが一気に増えます。体感改善に必要なのは隣接画像だけなので、少なくとも前後 1 枚ずつ、または件数上限付きにしたいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modal/FileModal/FileModalImage.vue` around lines 89 - 97, The current watchEffect iterates all siblingImageIds and creates an Image for each, causing a spike of requests; change it to preload only the adjacent images (previous and next) relative to props.fileId (or at most a small fixed N neighbors) by finding the index of props.fileId in siblingImageIds and only calling new Image() with buildFilePath for siblingImageIds[index-1] and siblingImageIds[index+1] (or a bounded slice like slice(max(0,index-N), min(len,index+N+1))). Keep the same watchEffect but replace the full forEach with logic that computes the neighbor ids and skips undefined or equal ids before creating Image instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modal/FileModal/FileModalImage.vue`:
- Around line 7-13: Replace the invisible hover-only navigation divs in
FileModalImage.vue with semantic buttons so touch users can discover and operate
them: change the left/right nav <div> elements that call prevFile and nextFile
into <button type="button"> elements (keep `@click.stop`="prevFile"/"nextFile" and
the class bindings $style.navButton, $style.navLeft, $style.navRight), add
accessible attributes (aria-label like "Previous image"/"Next image" and ensure
focusability), and update CSS so visibility is not solely :hover — make the
controls visible on :focus-visible and for touch devices (e.g. pointer:coarse
media query) or remove opacity gating while keeping hover styling as
enhancement; also ensure hidden states remove pointer-events rather than leaving
invisible hit areas.
---
Nitpick comments:
In `@src/components/Modal/FileModal/FileModalImage.vue`:
- Around line 122-136: onKeyDown currently only ignores inputs and textareas,
causing ArrowLeft/ArrowRight to trigger image navigation even when focus is on
contenteditable or select elements; update the onKeyDown handler to first assert
e.target is an HTMLElement and skip handling when e.target.isContentEditable is
true or when e.target is an instance of HTMLSelectElement (in addition to
existing checks for HTMLInputElement and HTMLTextAreaElement) so that
nextFile()/prevFile() are not called while editing or selecting; keep
useEventListener('keydown', onKeyDown) as-is.
- Around line 89-97: The current watchEffect iterates all siblingImageIds and
creates an Image for each, causing a spike of requests; change it to preload
only the adjacent images (previous and next) relative to props.fileId (or at
most a small fixed N neighbors) by finding the index of props.fileId in
siblingImageIds and only calling new Image() with buildFilePath for
siblingImageIds[index-1] and siblingImageIds[index+1] (or a bounded slice like
slice(max(0,index-N), min(len,index+N+1))). Keep the same watchEffect but
replace the full forEach with logic that computes the neighbor ids and skips
undefined or equal ids before creating Image instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d830822b-93a1-466d-b96e-e6ba985b828f
📒 Files selected for processing (1)
src/components/Modal/FileModal/FileModalImage.vue
Comment on lines
+7
to
+13
| <div | ||
| v-if="siblingImageIds.length > 1" | ||
| :class="[$style.navButton, $style.navLeft]" | ||
| @click.stop="prevFile" | ||
| > | ||
| <AIcon name="chevron-left" mdi :size="32" /> | ||
| </div> |
There was a problem hiding this comment.
タッチ端末では発見可能な遷移 UI がなくなっています。
表示条件が :hover 依存なので、hover のない端末では左右ボタンが見えません。しかも opacity: 0 のままでも hit area は残るので、見えないタップ領域だけが左右端に残ります。この PR では swipe を入れていないので、モバイルだと前後移動の手段が実質失われます。div ではなく <button> にして、非 hover 端末と :focus-visible では常に操作可能にしたいです。
🔧 修正案
- <div
+ <button
+ type="button"
+ aria-label="前の画像へ"
v-if="siblingImageIds.length > 1"
:class="[$style.navButton, $style.navLeft]"
`@click.stop`="prevFile"
>
<AIcon name="chevron-left" mdi :size="32" />
- </div>
+ </button>
...
- <div
+ <button
+ type="button"
+ aria-label="次の画像へ"
v-if="siblingImageIds.length > 1"
:class="[$style.navButton, $style.navRight]"
`@click.stop`="nextFile"
>
<AIcon name="chevron-right" mdi :size="32" />
- </div>
+ </button> .navButton {
`@include` color-ui-secondary;
position: absolute;
top: 50%;
transform: translateY(-50%);
z-index: $z-index-file-modal-header;
cursor: pointer;
padding: 4px;
border-radius: 50%;
+ border: 0;
+ color: inherit;
+ appearance: none;
background: rgba(0, 0, 0, 0.3);
display: flex;
align-items: center;
justify-content: center;
transition: all 0.2s ease;
opacity: 0;
+ pointer-events: none;
.container:hover & {
opacity: 1;
+ pointer-events: auto;
}
+
+ &:focus-visible {
+ opacity: 1;
+ pointer-events: auto;
+ }
+
+ `@media` (hover: none), (pointer: coarse) {
+ opacity: 1;
+ pointer-events: auto;
+ }
&:hover {
background: rgba(0, 0, 0, 0.6);
}
}Also applies to: 33-39, 192-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modal/FileModal/FileModalImage.vue` around lines 7 - 13,
Replace the invisible hover-only navigation divs in FileModalImage.vue with
semantic buttons so touch users can discover and operate them: change the
left/right nav <div> elements that call prevFile and nextFile into <button
type="button"> elements (keep `@click.stop`="prevFile"/"nextFile" and the class
bindings $style.navButton, $style.navLeft, $style.navRight), add accessible
attributes (aria-label like "Previous image"/"Next image" and ensure
focusability), and update CSS so visibility is not solely :hover — make the
controls visible on :focus-visible and for touch devices (e.g. pointer:coarse
media query) or remove opacity gating while keeping hover styling as
enhancement; also ensure hidden states remove pointer-events rather than leaving
invisible hit areas.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
画像を動かせる機能と競合するのでスワップは実装してないです
なぜこの PR を入れたいのか
UI 変更部分のスクリーンショット
PR を出す前の確認事項
Summary by CodeRabbit
リリース ノート