fix(compare): horizontally scrollable comparison table on mobile - #1735
shashwat-28410 wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe comparison display updates mobile cards with horizontal scrolling, snap alignment, reordering, refreshed styling, and compact nutrient values. It also changes the Nova Group accessibility label and adjusts desktop table width and product column sizing. ChangesComparison layout
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
@teolemon check this i have added the horizontal scrolling |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/ui/PwaInstallButton.svelte (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd default fallbacks to all new PWA translation calls.
The new PWA UI calls
$_()without the requireddefaultoption. Add the English fallback at every call site.
src/lib/ui/PwaInstallButton.svelte#L18-L18: add the success-message fallback.src/lib/ui/PwaInstallButton.svelte#L44-L44: add the success-message fallback.src/lib/ui/PwaInstallButton.svelte#L50-L50: add the error-message fallback.src/lib/ui/PwaInstallButton.svelte#L61-L61: add fallbacks for both button labels.src/routes/settings/+page.svelte#L230-L232: add fallbacks for the label and description.As per coding guidelines, “All user-facing strings must go through
svelte-i18n, and calls must include adefaultfallback.”🤖 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 `@src/lib/ui/PwaInstallButton.svelte` at line 18, Add the appropriate English default fallback to every new PWA translation call using $_: both success messages, the error message, and both button labels in PwaInstallButton.svelte, plus the settings label and description in src/routes/settings/+page.svelte at lines 230-232. Update the call sites in PwaInstallButton.svelte at lines 18, 44, 50, and 61, preserving the existing translation keys and UI behavior.Source: Coding guidelines
🤖 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 `@src/lib/ui/PwaInstallButton.svelte`:
- Around line 43-48: Update the installation completion handling in
PwaInstallButton around the userChoice outcome and appinstalled listener so
accepted installations do not trigger duplicate success toasts; coordinate or
deduplicate both success paths while preserving the dismissed behavior and
prompt clearing.
---
Nitpick comments:
In `@src/lib/ui/PwaInstallButton.svelte`:
- Line 18: Add the appropriate English default fallback to every new PWA
translation call using $_: both success messages, the error message, and both
button labels in PwaInstallButton.svelte, plus the settings label and
description in src/routes/settings/+page.svelte at lines 230-232. Update the
call sites in PwaInstallButton.svelte at lines 18, 44, 50, and 61, preserving
the existing translation keys and UI behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7393939-eefe-45cd-b625-472b3b71268a
📒 Files selected for processing (7)
src/lib/i18n/messages/en.jsonsrc/lib/stores/pwa.tssrc/lib/ui/ComparisonDisplay.sveltesrc/lib/ui/PwaInstallButton.sveltesrc/routes/+layout.sveltesrc/routes/settings/+page.sveltesrc/routes/site.webmanifest/+server.ts
| if (outcome === 'accepted') { | ||
| toastCtx.success($_('pwa.install_success')); | ||
| } | ||
| // Whether accepted or dismissed, clear the stored prompt | ||
| // — the browser won't fire `beforeinstallprompt` again. | ||
| pwaInstallStore.clear(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching PwaInstallButton:\n'
fd -a 'PwaInstallButton\.svelte$' . || true
printf '\nRelevant file outline:\n'
file="$(fd 'PwaInstallButton\.svelte$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" || true
printf '\nFile contents with line numbers:\n'
cat -n "$file"
fi
printf '\nSearch for pwaInstallStore / handleAppInstalled / install_success references:\n'
rg -n "pwaInstallStore|handleAppInstalled|install_success|appinstalled|userChoice|beforeinstallprompt" .Repository: openfoodfacts/openfoodfacts-explorer
Length of output: 4605
🌐 Web query:
MDN BeforeInstallPromptEvent appinstalled event userChoice accepted developer.mozilla.org
💡 Result:
The BeforeInstallPromptEvent, userChoice, and appinstalled event are related components of the Progressive Web App (PWA) installation process, primarily supported in Chromium-based browsers [1][2]. BeforeInstallPromptEvent and userChoice When a browser detects that a web app is installable, it fires a beforeinstallprompt event [3]. If a developer catches this event (and calls event.preventDefault() to suppress the default browser prompt), they can save the event object and trigger a custom installation prompt later by calling the prompt() method [1][3]. The prompt() method returns a Promise that resolves with a userChoice object [4][2]. This object contains: - outcome: A string that is either "accepted" (the user clicked to install) or "dismissed" (the user closed the prompt) [5][4]. - platform: A string indicating the selected platform if the user accepted the installation [5][6]. appinstalled Event While userChoice allows developers to track the outcome of a specific prompt triggered via their own UI, it does not cover installations triggered through other browser UI mechanisms (e.g., the browser's address bar icon) [7]. The appinstalled event is the recommended way to track successful installations regardless of how they were initiated [7][8]. This event fires on the global window object as soon as the browser has successfully installed the page as an application [9]. It is commonly used by developers to hide custom in-app install promotions once the application is already installed [1][8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/How_to/Trigger_install_prompt
- 2: https://developer.mozilla.org/en-US/docs/Web/API/BeforeInstallPromptEvent
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeinstallprompt_event
- 4: https://developer.mozilla.org/en-US/docs/Web/API/BeforeInstallPromptEvent/prompt
- 5: https://developer.mozilla.org/en-US/docs/Web/API/BeforeInstallPromptEvent/userChoice
- 6: https://github.com/mdn/content/blob/main/files/en-us/web/api/beforeinstallpromptevent/userchoice/index.md?plain=1
- 7: https://web.dev/articles/customize-install
- 8: https://web.dev/learn/pwa/detection
- 9: https://developer.mozilla.org/en-US/docs/Web/API/Window/appinstalled_event
🌐 Web query:
Chrome appinstalled event fires after beforeinstallprompt userChoice resolved accepted duplicate appinstalled
💡 Result:
The appinstalled event and the userChoice promise from the beforeinstallprompt event serve distinct purposes and are triggered by different stages of the installation process [1][2]. The userChoice promise resolves as soon as the user responds to the browser's install dialog (e.g., clicking "Install" or "Cancel") [3][2]. It provides the outcome of that specific interaction (either 'accepted' or 'dismissed') [3][4]. The appinstalled event fires only after the browser has successfully completed the installation of the Progressive Web App (PWA) [5]. While the userChoice promise indicates the user's intent to install, the appinstalled event serves as confirmation that the installation has actually occurred [1][6]. Because these events track different points in the lifecycle, appinstalled typically fires after the userChoice promise has resolved with an 'accepted' outcome, provided the installation succeeds [1][5]. Note that the appinstalled event is intended to fire regardless of the installation mechanism—whether triggered by your own UI via prompt or by the browser's own automated prompts [1][7]. Developers should use userChoice to track user intent and analytics, and use appinstalled to confirm the successful installation of the application [1][2].
Citations:
- 1: https://web.dev/articles/customize-install
- 2: https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/How_to/Trigger_install_prompt
- 3: https://developer.mozilla.org/en-US/docs/Web/API/BeforeInstallPromptEvent/userChoice
- 4: https://developer.chrome.com/blog/app-install-banners-native
- 5: https://www.w3.org/TR/2019/WD-appmanifest-20190925/
- 6: https://stackoverflow.com/questions/60320837/appinstalled-event-is-fired-prematurely
- 7: https://web.dev/learn/pwa/installation-prompt
Prevent duplicate installation success toasts.
userChoice resolves with accepted before window.appinstalled confirms installation. Show the success toast on only one path, or coalesce both success paths so the user sees one completion message.
🤖 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 `@src/lib/ui/PwaInstallButton.svelte` around lines 43 - 48, Update the
installation completion handling in PwaInstallButton around the userChoice
outcome and appinstalled listener so accepted installations do not trigger
duplicate success toasts; coordinate or deduplicate both success paths while
preserving the dismissed behavior and prompt clearing.
|
Your screenshot shows compacted columns that are not practical to show information (ingredients, nutrition). The point of horizontal scrolling is not to compact the info to the point where it becomes unlegible |
Screen.Recording.2026-08-06.at.1.56.38.PM.movcheck this i made it better to read and applied horizontal scrolling |
|
dd9cad3 to
e89d819
Compare
Screen.Recording.2026-08-06.at.6.43.57.PM.mov |
|
@shashwat-28410 did you just use the agent and then send the PR? Or did you have a look at the code it produced? Do you know how it works? |
|
@VaiTon I did review the code — I went through the implementation line by line before pushing. The agent helped me get oriented |
|
I mean — it looks you haven't even reviewed the code. |
|
@shashwat-28410 I recommend reviewing the diff of the PR after creating it. This PR now has some changes related to PWA. Please remove them. |
Replace the vertical card-based mobile compare view with the existing responsive table (already used on desktop) across all screen sizes. The table has overflow-x-auto and a sticky first column so nutrient labels stay pinned while users swipe left/right to compare products side by side. This directly addresses teolemon's feedback on openfoodfacts#1722: 'The real plus on compare mode on mobile would be horizontal scrolling'. Also removes the now-unused nutrientValue snippet (only used by the removed card view). Fixes openfoodfacts#1722
… mobile Adds a content-derived min-width (10rem sticky col + 12rem per product) to the comparison table so columns never compress below a readable size. Also pins product column headers to w-48. The overflow-x-auto wrapper scrolls horizontally when the table exceeds the viewport. Addresses founder feedback that compacted columns were illegible for ingredients and nutrition information.
Replace the cramped table-on-mobile layout with a horizontal card strip where each product is a distinct, spacious card arranged side by side in a snap-scroll container. Products are 'stacked horizontally' with generous gap (1rem breathing space) and swipeable left/right to reveal all products. This directly addresses founder feedback: products should be stacked horizontally with breathing space and horizontal scroll — the table's sticky label column (160px) on a 375px viewport made the scroll feel useless and products appear 'stacked vertically.' Desktop (lg+) keeps the existing responsive comparison table unchanged.
e89d819 to
936bf16
Compare
|
@VaiTon Done PWA commits removed. The PR now contains only the compare horizontal scroll changes.And i will make sure reviewing the diff next time. |
Replaces the vertical card-based mobile compare view with the
existing responsive table (already used on desktop) across all
screen sizes. The table already has
overflow-x-autoand astickyfirst column, so on mobile users can swipe left/rightto compare products side by side while nutrient labels stay pinned.
This directly addresses teolemon's feedback on #1722: "The real
plus on compare mode on mobile would be horizontal scrolling."
What changed:
lg:hidden/lg:blocksplit — the table now rendersat all screen sizes.
nutrientValuesnippet (was only used by thecard view; the table uses
nutrientValueDesktop).Net result: -130 lines, one unified responsive table.
Screenshot or video
Related issue(s) and discussion
Follow-up to the review discussion on PR #1722.
Checklist: Author Self-Review
Large Language Models usage disclosure
Summary by CodeRabbit
New Features
Accessibility