883-feat: Make swipe gallery for merch card #889
Conversation
chmdln
requested review from
andron13,
dzmitry-varabei,
SpaNb4,
Quiddlee,
ansivgit and
natanchik
as code owners
github-actions
bot
changed the title
📝 WalkthroughWalkthroughThis update introduces a new "Merch" section, including backend API integration, data transformation utilities, React components, and styling for a merchandise catalog and swipeable product cards. It also updates routing, constants, and breadcrumbs to support the new merch feature, along with related dependency and test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MerchPage
participant MerchCatalog
participant MerchStore
participant API
participant Backend
User->>MerchPage: Navigate to /merch
MerchPage->>MerchCatalog: Render
MerchCatalog->>MerchStore: loadMerchCatalog()
MerchStore->>API: queryMerchCatalog()
API->>Backend: GET merch/filelist.json
Backend-->>API: MerchResponse
API-->>MerchStore: MerchResponse
MerchStore->>MerchCatalog: Transformed products
MerchCatalog->>User: Display MerchList (grid of MerchCards)
User->>MerchCard: Swipe or click arrows
MerchCard->>User: Show next/prev product image
Assessment against linked issues
Suggested labels
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
package.json (1)
41-41: Ensure dependency usage and compatibility.
Addedreact-swipeable,swiper, anduuid(with types). Please verify that:
swiperis actually utilized; if not, consider removing it.react-swipeableintegration aligns with the component implementation.uuidimport and@types/uuidversions are compatible.Also applies to: 50-52, 69-69
src/widgets/merch-catalog/ui/merch-catalog.module.scss (1)
1-4: Add layout refinements for item spacing.
Consider adding properties likegapandflex-wrapto improve the merchandising layout and ensure responsiveness.src/widgets/merch-catalog/ui/merch-list/merch-list.module.scss (1)
5-9: Consider adding responsive breakpointsThe grid layout looks good, but consider adding media queries to handle different screen sizes for better mobile experience.
.list { display: grid; grid-template-columns: repeat(3, 1fr); gap: 24px; + + @media (max-width: 768px) { + grid-template-columns: repeat(2, 1fr); + } + + @media (max-width: 480px) { + grid-template-columns: 1fr; + } }src/entities/merch/helpers/transform-merch-catalog.ts (1)
32-36: Enhance type guard validation.The type guard checks for property existence but not data validity. Consider adding deeper validation for array properties.
src/entities/merch/ui/merch-card/merch-card.tsx (1)
68-77: Remove redundant conditional check.The condition at line 68 (
preview.length > 1) is redundant as it's already covered by the parent conditional at line 47.src/entities/merch/ui/merch-card/merch-card.module.scss (5)
50-58: Specify flex basis in shorthand
You’ve usedflex: 1 1;on.info-wrap, which defaults the flex-basis to0%. If you intend to preserve content size before shrinking, considerflex: 1 1 auto;or simplyflex: 1;for clarity.
66-81: Add focus styles for accessibility
Interactive elements like.downloadbuttons should include visible focus states (e.g.:focus-visible { outline: 2px solid $color-primary; }) to improve keyboard navigation.
83-88: Review.download-imgbackground-color
A class named.download-imgsuggests an<img>element, but you’re applying abackground-color. If this is truly an image tag, the background won’t show—consider using a<div>or renaming the class, or remove the redundant background.
90-136: Enhance swipe-button accessibility
For.swipe-btn, consider adding:focus-visibleoutlines,aria-labelstyling hooks, and perhapspointer-events: none;on disabled buttons to reinforce the disabled state.
163-172: Improve swipeable area behavior
Consider addingoverflow-x: hidden;anduser-select: none;to.swipeable-areato prevent off-screen items from peeking and to avoid accidental text selection during swipes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/shared/assets/svg/chevron-left-solid.svgis excluded by!**/*.svgsrc/shared/assets/svg/chevron-right-solid.svgis excluded by!**/*.svgsrc/shared/assets/svg/download.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
dev-data/hero-page.data.ts(2 hunks)package.json(3 hunks)src/app/merch/page.tsx(1 hunks)src/core/api/app-api.ts(2 hunks)src/core/styles/_constants.scss(1 hunks)src/entities/merch/api/merch-api.ts(1 hunks)src/entities/merch/helpers/transform-merch-catalog.ts(1 hunks)src/entities/merch/index.ts(1 hunks)src/entities/merch/model/store.ts(1 hunks)src/entities/merch/types.ts(1 hunks)src/entities/merch/ui/merch-card/merch-card.module.scss(1 hunks)src/entities/merch/ui/merch-card/merch-card.tsx(1 hunks)src/shared/__tests__/constants.ts(1 hunks)src/shared/constants.ts(2 hunks)src/shared/ui/paragraph/paragraph.module.scss(1 hunks)src/views/merch/merch.tsx(1 hunks)src/widgets/breadcrumbs/constants.ts(1 hunks)src/widgets/merch-catalog/index.ts(1 hunks)src/widgets/merch-catalog/ui/merch-catalog.module.scss(1 hunks)src/widgets/merch-catalog/ui/merch-catalog.tsx(1 hunks)src/widgets/merch-catalog/ui/merch-list/merch-list.module.scss(1 hunks)src/widgets/merch-catalog/ui/merch-list/merch-list.tsx(1 hunks)src/widgets/merch/ui/merch.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/views/merch/merch.tsx (2)
src/shared/constants.ts (1)
PAGE_NAMES(25-31)src/widgets/merch-catalog/ui/merch-catalog.tsx (1)
MerchCatalog(10-20)
src/core/api/app-api.ts (3)
src/entities/merch/api/merch-api.ts (1)
MerchApi(4-10)src/shared/api/api-base-class.ts (1)
ApiBaseClass(14-127)src/entities/trainer/api/trainer-api.ts (1)
TrainerApi(6-19)
src/entities/merch/api/merch-api.ts (1)
src/entities/merch/types.ts (1)
MerchResponse(16-18)
src/entities/merch/helpers/transform-merch-catalog.ts (1)
src/entities/merch/types.ts (4)
MerchResponse(16-18)MerchProduct(20-27)ApiMerchItemAdapt(15-15)ApiMerchItem(1-5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (20)
src/shared/ui/paragraph/paragraph.module.scss (1)
2-4: Validate new top padding across layouts.
Thepadding-top: 4px;may affect existing paragraph spacing. Please confirm it doesn’t introduce visual regressions, especially in shared components.src/core/styles/_constants.scss (1)
85-85: Sync SCSS constants with e2e viewports.
You added$opacity-40. The file header notes updatingviewports.tsine2e/utilsafter changing variables—please ensure those configs remain in sync.src/widgets/breadcrumbs/constants.ts (1)
16-16: Confirm breadcrumb and route integration.
With'merch': 'Merch'added, verify that the/merchroute exists and that theBreadcrumbNameMaptype includes this key to prevent type errors.src/widgets/merch-catalog/index.ts (1)
1-2: Clean barrel export - looks good!This is a proper barrel file that exports the MerchCatalog component, following the project's module structure pattern.
src/shared/constants.ts (2)
30-30: Consistent route naming - approvedAdding MERCH to PAGE_NAMES maintains the established naming pattern and supports the new merch feature.
105-105: Route definition looks goodAdding MERCH to ROUTES ensures consistent routing for the new merch page throughout the application.
src/shared/__tests__/constants.ts (1)
214-214: Correctly updated link to use internal routingChanged from external URL to relative path, aligning with the new internal merch page routing structure.
src/entities/merch/index.ts (1)
1-3: Well-structured exports for the merch entityThis barrel file properly exports the essential components for the merch feature: the type definition, UI component, and data store.
src/widgets/merch/ui/merch.tsx (2)
5-5: Correctly updated import to use ROUTESChanged from LINKS to ROUTES import to support the new internal routing strategy.
24-26: Properly updated link to use internal routingThe link now correctly uses the ROUTES constant with template string formatting and appropriately removes the external prop.
src/views/merch/merch.tsx (1)
6-14: Clean and well-structured component implementationThe
Merchcomponent follows a good pattern of composing a page from multiple specialized components. The async declaration is appropriate since it renders the asyncMerchCatalogcomponent.src/core/api/app-api.ts (1)
1-1: API integration follows established patternsThe merch API integration is properly implemented following the same pattern as existing API modules in the codebase.
Also applies to: 11-11, 18-18
src/entities/merch/api/merch-api.ts (1)
4-10: Clean, focused API implementation.The MerchApi class provides a single responsibility of fetching merchandise data using a REST GET request. The implementation follows good practices with dependency injection via constructor.
dev-data/hero-page.data.ts (1)
30-36: Hero data structure for merch section looks good.The new merch section follows the same structure as other sections with appropriate title, subtitle, and image properties.
src/widgets/merch-catalog/ui/merch-list/merch-list.tsx (1)
1-21: Well-structured component with clean implementation.The MerchList component efficiently renders the grid of merchandise cards with proper key usage. The implementation is straightforward and follows React best practices.
src/entities/merch/types.ts (1)
1-27: Type definitions are comprehensive and well-structured.The types clearly model the hierarchical structure of merchandise data and provide good type safety throughout the application.
src/entities/merch/ui/merch-card/merch-card.tsx (1)
19-39: Good state management and swipe implementation.The component effectively manages state for the image carousel with clean navigation functions.
src/entities/merch/ui/merch-card/merch-card.module.scss (3)
15-23: Verify CSS Color Module Level 4 support
This file uses modern color functions (hsla(from $color-black h s l / ...)and slash alpha notation likergb(0 0 0 / 30%)). Ensure your build pipeline (Dart Sass, PostCSS preset-env, autoprefixer, etc.) supports CSS Color Module Level 4, or provide fallbacks for older browsers.Also applies to: 43-46
60-64: Image styling looks good
width: 100%; height: 100%; object-fit: contain;ensures images scale correctly within the card. Nice work!
174-178: Responsive touch layout approved
Hiding.swipe-btnunder themax-width: $mobile-landscape-widthbreakpoint makes sense for touch devices. Good use of media queries!
| export const MerchCatalog = async () => { | ||
| const products = await merchStore.loadMerchCatalog(); | ||
|
|
||
| return ( | ||
| <section className={cx('container')}> | ||
| <div className={cx('content', 'merch-catalog')}> | ||
| {products && <MerchList products={products} />} | ||
| </div> | ||
| </section> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error and loading states.
The async component loads data but doesn't handle loading states or display errors to users if the API call fails.
export const MerchCatalog = async () => {
const products = await merchStore.loadMerchCatalog();
return (
<section className={cx('container')}>
<div className={cx('content', 'merch-catalog')}>
- {products && <MerchList products={products} />}
+ {products ? (
+ <MerchList products={products} />
+ ) : (
+ <div className={cx('error-message')}>
+ Unable to load merchandise. Please try again later.
+ </div>
+ )}
</div>
</section>
);
};Committable suggestion skipped: line range outside the PR's diff.
src/entities/merch/model/store.ts
Outdated
| public loadMerchCatalog = async () => { | ||
| try { | ||
| const res = await api.merch.queryMerchCatalog(); | ||
|
|
||
| if (res.isSuccess) { | ||
| return transformMerchCatalog(res.result); | ||
| } | ||
| } catch (e) { | ||
| console.error(e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in loadMerchCatalog method.
Currently, the method returns undefined when an error occurs, which could lead to confusing UI states. Consider returning an empty array or error state that components can properly handle.
public loadMerchCatalog = async () => {
try {
const res = await api.merch.queryMerchCatalog();
if (res.isSuccess) {
return transformMerchCatalog(res.result);
}
+ return []; // Return empty array if request was not successful
} catch (e) {
console.error(e);
+ return []; // Return empty array on error
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public loadMerchCatalog = async () => { | |
| try { | |
| const res = await api.merch.queryMerchCatalog(); | |
| if (res.isSuccess) { | |
| return transformMerchCatalog(res.result); | |
| } | |
| } catch (e) { | |
| console.error(e); | |
| } | |
| }; | |
| public loadMerchCatalog = async () => { | |
| try { | |
| const res = await api.merch.queryMerchCatalog(); | |
| if (res.isSuccess) { | |
| return transformMerchCatalog(res.result); | |
| } | |
| return []; // Return empty array if request was not successful | |
| } catch (e) { | |
| console.error(e); | |
| return []; // Return empty array on error | |
| } | |
| }; |
| export const transformMerchCatalog = (data: MerchResponse): MerchProduct[] => { | ||
| const products: MerchProduct[] = []; | ||
| const baseUrl = process.env.API_BASE_URL; | ||
| const processCategory = (category: ApiMerchItemAdapt, parentTags: string[]) => { | ||
| for (const [key, value] of Object.entries(category)) { | ||
| if (isApiMerchItem(value)) { | ||
| products.push({ | ||
| id: uuidv4(), | ||
| name: key, | ||
| title: value.name, | ||
| preview: value.preview.map((path) => `${baseUrl}/${path}`), | ||
| download: value.download.map((path) => `${baseUrl}/${path}`), | ||
| tags: parentTags, | ||
| }); | ||
| } else { | ||
| processCategory(value, [...parentTags, key]); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| for (const [categoryName, categoryData] of Object.entries(data)) { | ||
| processCategory(categoryData, [categoryName]); | ||
| } | ||
|
|
||
| return products; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve URL construction.
The transformation function works well for flattening the nested catalog structure, but lacks error handling and proper URL construction. Consider these improvements:
- Add validation for
baseUrlto ensure it's defined - Implement URL joining logic to prevent double slashes if
baseUrlends with a slash or path starts with one - Add try-catch blocks to handle errors during transformation
| background-color: rgb(0 0 0 / 20%); | ||
|
|
||
| transition: background-color all 0.3s ease; | ||
|
|
There was a problem hiding this comment.
Fix transition property syntax
transition: background-color all 0.3s ease; is invalid. It should be either:
transition: background-color 0.3s ease;or
transition: all 0.3s ease;Please correct this to ensure the active dot transitions smoothly.
There was a problem hiding this comment.
It's good point, please fix it
There was a problem hiding this comment.
No description provided.
There was a problem hiding this comment.
A summary of the edits CodeRabbit can apply:
Fix the transition property syntax by removing the invalid "all" keyword in src/entities/merch/ui/merch-card/merch-card.module.scss
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):
✏️ src/entities/merch/ui/merch-card/merch-card.module.scss
Open src/entities/merch/ui/merch-card/merch-card.module.scss and navigate to the .swipe-dot block. Locate the line:
transition: background-color all 0.3s ease;
Replace it exactly with:
transition: background-color 0.3s ease;
Save the file, restart your SCSS compiler or development server, and verify in the browser that the swipe dots’ background-color now transitions smoothly without any syntax errors.
A summary of the context that CodeRabbit is considering across the codebase:
We’ll use ripgrep to locate the invalid transition syntax and confirm its exact line number and surrounding context before editing.
-
✅ Create PR with these edits
-
📌 Create commit in current branch
-
📋 Get copyable edits
| &::after { | ||
| pointer-events: none; | ||
| content: ''; | ||
|
|
||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
|
|
||
| width: 100%; | ||
| height: 100%; | ||
|
|
||
| background: linear-gradient( | ||
| to bottom, | ||
| transparent 0%, | ||
| hsla(from $color-black h s l / $opacity-10) 100% | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit z-index to gradient overlay
The &::after pseudo-element in .preview-wrap creates a gradient overlay but has no z-index. Without it, stacking of images, buttons, or dots may be inconsistent. Add z-index: 1; to the pseudo-element and ensure interactive elements (e.g. swipe buttons) sit above it (z-index: 2;).
| .merch-card { | ||
| position: relative; | ||
|
|
||
| overflow: hidden; |
There was a problem hiding this comment.
Overflow hidden may clip pagination dots
.merch-card { overflow: hidden; } will clip any absolutely-positioned .swipe-dots placed outside its bounds (e.g. bottom: -15px). Consider adjusting overflow to visible on the lower section or repositioning dots within the container.
| @@ -38,6 +38,7 @@ | |||
| "react-double-marquee": "^1.1.0", | |||
| "react-markdown": "^10.1.0", | |||
| "react-responsive-carousel": "^3.2.23", | |||
| "react-swipeable": "^7.0.2", | |||
There was a problem hiding this comment.
Is there a reason we're using react-swipeable here instead of Swiper, given that Swiper is already listed in the dependencies?
There was a problem hiding this comment.
@gbogdanova good point, since we also have the following issue: #877
| @@ -65,6 +66,7 @@ | |||
| "@types/node": "^22.15.3", | |||
| "@types/react": "19.1.2", | |||
| "@types/react-dom": "19.1.3", | |||
| "@types/uuid": "^10.0.0", | |||
There was a problem hiding this comment.
This library was used in the 786 branch but it has now been removed. Try pulling the changes from branch 786 again, so that you have the current version and don't add unnecessary dependencies
| import { useSwipeable } from 'react-swipeable'; | ||
|
|
||
| import { MerchProduct } from '@/entities/merch/types'; | ||
| import chevronLeft from '@/shared/assets/svg/chevron-left-solid.svg'; | ||
| import chevronRight from '@/shared/assets/svg/chevron-right-solid.svg'; |
There was a problem hiding this comment.
The Slider component uses the swiper library, can we reuse it? And maybe then we won't need new arrow icons?
| {preview.length > 1 && ( | ||
| <> | ||
| <button className={cx('swipe-btn', 'is-prev')} onClick={goToPrev} disabled={currentIndex === 0}> | ||
| <Image | ||
| src={chevronLeft} | ||
| alt="chevron left icon" | ||
| width={20} | ||
| height={20} | ||
| className={cx('arrow-img')} | ||
| /> | ||
| </button> | ||
| <button className={cx('swipe-btn', 'is-next')} onClick={goToNext} disabled={currentIndex === preview.length - 1}> | ||
| <Image | ||
| src={chevronRight} | ||
| alt="chevron right icon" | ||
| width={20} | ||
| height={20} | ||
| className={cx('arrow-img')} | ||
| /> | ||
| </button> | ||
|
|
||
| {preview.length > 1 && ( | ||
| <div className={cx('swipe-dots')}> | ||
| {preview.map((_, index) => ( | ||
| <span | ||
| key={index} | ||
| className={cx('swipe-dot', { active: index === currentIndex })} | ||
| /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Why we make check two times?
{preview.length > 1....
| @@ -1,5 +1,6 @@ | |||
| .paragraph { | |||
| margin: 0; | |||
| padding-top: 4px; | |||
There was a problem hiding this comment.
This component is used in the whole project, this will change the markup of all components that use it. If you need to separate the text from the slider, use gap
| background-color: rgb(0 0 0 / 20%); | ||
|
|
||
| transition: background-color all 0.3s ease; | ||
|
|
There was a problem hiding this comment.
It's good point, please fix it
| font-size: 20px; | ||
| color: $color-white; | ||
|
|
||
| background-color: rgb(0 0 0 / 30%); |
There was a problem hiding this comment.
please use this syntax:
background-color: hsla(from $color-black h s l / $opacity-30);
| &.is-prev { | ||
| left: 8px; | ||
| } | ||
|
|
||
| &.is-next { | ||
| right: 8px; | ||
| } |
There was a problem hiding this comment.
lets make this
| &.is-prev { | |
| left: 8px; | |
| } | |
| &.is-next { | |
| right: 8px; | |
| } | |
| &.is-prev, | |
| &.is-next { | |
| left: 8px; | |
| } |
| } | ||
|
|
||
| &:hover { | ||
| background-color: rgb(0 0 0 / 60%); |
|
|
||
| &:disabled { | ||
| cursor: default; | ||
| opacity: 0.3; |
| height: 8px; | ||
| border-radius: 50%; | ||
|
|
||
| background-color: rgb(0 0 0 / 20%); |
What type of PR is this? (select all that apply)
Description
Add swipe gallery for merch card.
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
New Features
Style
Bug Fixes
Chores