feat: redesign mobile key factor overlay as swipeable pager#4952
Open
aseckin wants to merge 10 commits into
Open
feat: redesign mobile key factor overlay as swipeable pager#4952aseckin wants to merge 10 commits into
aseckin wants to merge 10 commits into
Conversation
Rebuild the narrow-screen key factor experience as a clean, dvh-bounded horizontal pager: - Embla-based pager: horizontal swipe (and prominent bottom arrows + dot indicators) switches between key factors; vertical drag scrolls the comment within a slide. Slides run edge-to-edge full-bleed. - Layout fits the dynamic viewport: shared "KEY FACTORS FOR" + question title + CP header, key factor card, and a comment region that fills the remaining height. Reply/upvote/changed-my-mind sit at the end of the comment so they appear only after scrolling. - Key factor text rendered larger via a new `large` variant on KeyFactorItem (driver, base rate, news), matching the comment username. Also fixes overlays rendering behind the navbar at the source: add `header`/`modal` z-index tokens and lift the shared BaseModal + key factor overlays above the sticky top chrome so every modal covers the navbar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaces the manual carousel in ChangesKey Factor Carousel Refactor and Visual Updates
Sequence DiagramsequenceDiagram
participant KeyFactorDetailOverlay
participant MobileKeyFactorOverlay
participant EmblaCarousel
participant useCommentsFeed
KeyFactorDetailOverlay->>MobileKeyFactorOverlay: allPostKeyFactors, initialIndex, onSelectKeyFactor
MobileKeyFactorOverlay->>EmblaCarousel: init with startIndex
EmblaCarousel->>MobileKeyFactorOverlay: select/reInit → selectedIndex, canPrev, canNext
MobileKeyFactorOverlay->>useCommentsFeed: ensureCommentLoaded(kf.comment_id)
useCommentsFeed-->>MobileKeyFactorOverlay: comment data
MobileKeyFactorOverlay->>KeyFactorDetailOverlay: onSelectKeyFactor(keyFactor)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/app/(main)/questions/[id]/components/key_factors/key_factor_detail_overlay.tsx (1)
138-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the desktop loading state out of the “simple” branch.
isSimplenow treatscomment === nullthe same as “loaded empty comment”, so a driver key factor with no siblings renders the card-only modal first and only switches toCommentDetailPanelafterensureCommentLoaded()resolves. That drops the existing loading/skeleton path during slow fetches.💡 Suggested fix
+ const hasComment = !!(comment?.text?.trim() || !comment); const isSimple = questionLink || !keyFactor?.driver || - (relatedKeyFactors.length === 0 && !comment?.text?.trim()); + (relatedKeyFactors.length === 0 && !hasComment);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/app/`(main)/questions/[id]/components/key_factors/key_factor_detail_overlay.tsx around lines 138 - 141, The `isSimple` condition in `KeyFactorDetailOverlay` is too broad because it treats an unloaded `comment` the same as an empty comment, which forces the card-only modal while `ensureCommentLoaded()` is still pending. Update the branching so the “simple” path only applies when the comment has actually loaded and is empty, and keep the existing loading/skeleton state for unresolved comments. Use the `isSimple` logic and the `CommentDetailPanel`/loading branch in `key_factor_detail_overlay.tsx` to preserve the desktop loading behavior during slow fetches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsx:
- Around line 135-145: The mobile Reply action is incorrectly reusing
handleScrollToComment, so it only navigates to the comment thread instead of
starting the reply flow. Update the mobile key factor overlay handlers in
mobile_key_factor_overlay.tsx so onReply uses a dedicated reply handler,
matching the desktop behavior, and call
questionLayout?.requestReplyToComment(kf.comment_id) after ensureCommentLoaded
rather than requestScrollToComment. Keep the existing view-comment behavior in
handleScrollToComment unchanged.
- Around line 372-376: The carousel navigation accessibility labels in
mobile_key_factor_overlay are hardcoded English, so they bypass the existing
i18n flow. Update the button aria-labels in the carousel/key factor navigation
(including the repeated controls around the current key factor list) to use the
component’s existing useTranslations() helper instead of string literals, and
add the corresponding locale keys for the “Go to key factor …” and related
navigation labels.
---
Outside diff comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/key_factors/key_factor_detail_overlay.tsx:
- Around line 138-141: The `isSimple` condition in `KeyFactorDetailOverlay` is
too broad because it treats an unloaded `comment` the same as an empty comment,
which forces the card-only modal while `ensureCommentLoaded()` is still pending.
Update the branching so the “simple” path only applies when the comment has
actually loaded and is empty, and keep the existing loading/skeleton state for
unresolved comments. Use the `isSimple` logic and the
`CommentDetailPanel`/loading branch in `key_factor_detail_overlay.tsx` to
preserve the desktop loading behavior during slow fetches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb09de59-781c-45f4-8e66-c6357d8de80a
📒 Files selected for processing (16)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/components/top_chrome_client.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/base_rate/key_factor_base_rate.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/driver/key_factor_driver.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/index.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/news/key_factor_news.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/news/key_factor_news_item.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/key_factor_detail_overlay.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsxfront_end/src/components/base_modal.tsxfront_end/tailwind.config.ts
- Footer: single space-efficient row with prev arrow on the left edge, dot indicators centered, next arrow on the right edge. - First/last slides keep a screen-edge gap equal to the header padding (16px) instead of touching the edge; middle peeks still run edge to edge. - Comment header stacks username over timestamp (like the feed's mobile comment) so long usernames no longer break the layout; reuse formatUsername. - Scroll the whole key factor + comment slide as one unit, so swiping up scrolls the key factor away too, instead of only scrolling the comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace hardcoded "Previous"/"Next"/"Go to key factor N" aria-labels with
the existing useTranslations() helper. Reuse the existing `previous`/`next`
keys and add a new `goToKeyFactor` ({number}) key across all locale files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mobile overlay's Reply action reused handleScrollToComment, so it only navigated to the comment thread. Add a dedicated handleReplyToComment that calls questionLayout.requestReplyToComment after ensureCommentLoaded (matching desktop), and wire onReply to it. View-comment behavior (handleScrollToComment / onScrollToLink) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The slide scroll container used overflow-y-auto with overflow-x left visible, which CSS computes to auto — letting the content drift horizontally during a vertical swipe. Explicitly clip the x-axis with overflow-x-hidden. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Multiple key factors can share a comment, and the list is sorted by vote score, so swiping re-showed the same comment repeatedly. Lift the comment out of the horizontal carousel into a single shared panel below the key-factor pager: - The carousel now holds only the key factor cards; its height tracks the active card (manual embla auto-height) so the comment re-anchors below it. - The comment renders once for the active key factor. It is keyed by comment_id, so swiping between key factors of the SAME comment reuses it in place (static), while a different comment remounts and cross-fades (new animate-fade-in utility). - Pager + comment share one vertical scroll column, so swiping up still scrolls the card away. - Remove the in-comment "Key Factors" chips (siblings are reached by swiping the cards). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/app/(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsx (1)
320-340: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the dialog an accessible title. The visible
<h2>isn’t associated with theDialog, so the modal can be announced without a name. Wire it witharia-labelledbyor useDialogTitle.♿ Proposed fix
- <Dialog as="div" className="relative z-modal" onClose={onClose}> + <Dialog + as="div" + className="relative z-modal" + onClose={onClose} + aria-labelledby="mobile-key-factor-overlay-title" + > @@ - <h2 className="mt-0 min-w-0 flex-1 text-base font-semibold leading-5 tracking-tight text-gray-800 dark:text-gray-800-dark"> + <h2 + id="mobile-key-factor-overlay-title" + className="mt-0 min-w-0 flex-1 text-base font-semibold leading-5 tracking-tight text-gray-800 dark:text-gray-800-dark" + > {post.title} </h2>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/app/`(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsx around lines 320 - 340, The mobile key factor modal in mobile_key_factor_overlay.tsx lacks an accessible name because the visible post title is not linked to the Dialog. Update the Dialog in the overlay component to expose a title by connecting the existing heading (the h2 that renders post.title) with aria-labelledby, or replace it with DialogTitle so assistive technologies can announce the modal properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsx:
- Around line 320-340: The mobile key factor modal in
mobile_key_factor_overlay.tsx lacks an accessible name because the visible post
title is not linked to the Dialog. Update the Dialog in the overlay component to
expose a title by connecting the existing heading (the h2 that renders
post.title) with aria-labelledby, or replace it with DialogTitle so assistive
technologies can announce the modal properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b79c2021-c4b7-44ff-8243-56d1a377ce2e
📒 Files selected for processing (2)
front_end/src/app/(main)/questions/[id]/components/key_factors/mobile_key_factor_overlay.tsxfront_end/tailwind.config.ts
- Swipe to navigate: a horizontal swipe on the comment (not just the cards) now pages between key factors. - Edge gap: give the first/last cards a 16px screen-edge gap via per-slide first:pl-4/last:pr-4 (reliably measured by embla) instead of container padding, so the last card no longer hugs the right edge. - Active vs hover: disable card hover in the overlay (no sticky hover on touch) and instead highlight the centered card with an active blue-500 border (transition-colors, timed with the slide). The attached comment gets the same border, masked to fade out after 50px. - Scroll fades: add ~10px top/bottom gradient fade overlays on the inner scroll area, shown based on scroll position. - Directional comment transition: replace the flat crossfade with a slower (280ms) directional slide-in keyed off swipe direction (animate-comment-in-right / -left). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The comment panel is keyed by comment_id, so same-comment swipes keep it mounted. But the entrance animation class was derived from the live swipe direction, so reversing direction flipped the animation-name on the still -mounted panel and re-triggered the animation for a comment that hadn't changed. Freeze the entrance direction at the moment comment_id changes (adjust-state-during-render), so the class is stable while the same comment stays mounted and only changes on a real comment switch. Also give the overlay an accessible name: render the post title via Headless UI DialogTitle so the modal is announced by assistive tech. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Closes #4839
What & why
The narrow-screen key factor overlay (tap a key factor on mobile) was poorly designed: navigation wasn't swipeable or obvious, prev/next arrows were small and faint, the overlay rendered behind the navbar (close button could be unreachable), and content didn't fit small viewports.
Changes
Mobile overlay → dvh-bounded horizontal pager (full rewrite)
Navbar-overlap fix (app-wide)
header/modalz-index tokens; lifted shared BaseModal + key factor overlays above the sticky top chrome.Larger key-factor font
largevariant on KeyFactorItem (driver/base-rate/news), matching the comment username size.i18n: new
keyFactorsForkey in all six message files.Verification
ESLint + tsc clean. Not verified in-browser (local frontend can't reach the backend API here) — worth a manual pass on a real question with multiple key factors at a small mobile viewport.
Summary by CodeRabbit