-
Notifications
You must be signed in to change notification settings - Fork 78
refactor(composables): product listing #2048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…/frontends into feat/product-listing-ui
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactored the monolithic useListing composable into smaller, focused modules to improve modularity, maintainability, and SSR support for product listings. The main goal is to enable early data access to avoid ClientOnly loading without hydration mismatches.
- Split single large composable into focused, single-responsibility modules
- Added SSR optimization with early data extraction from CMS pages
- Improved filter component UI consistency and animations
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/helpers/src/cms/getProductListingFromCmsPage.ts | New helper to extract product listing data from CMS page structure for SSR optimization |
| packages/composables/src/useListing/useListingCore.ts | Core state management with provide/inject pattern and shared state |
| packages/composables/src/useListing/useProductListingProducts.ts | Products data and search operations module |
| packages/composables/src/useListing/useProductListingPagination.ts | Pagination logic module |
| packages/composables/src/useListing/useProductListingSorting.ts | Sorting functionality module |
| packages/composables/src/useListing/useProductListingFilters.ts | Filter management module |
| packages/composables/src/useListing/utils.ts | Shared types and utility functions |
| packages/composables/src/useListing/useListing.ts | Refactored facade that composes all modular composables |
| packages/cms-base-layer/app/components/public/cms/CmsPage.vue | Extract listing early and create context with initial listing |
| packages/cms-base-layer/app/components/SwProductListingFilters.vue | Removed ClientOnly wrappers and improved filter state management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| await core.search( | ||
| Object.assign( | ||
| { | ||
| page, |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property should be 'p' instead of 'page' to match the API parameter format used elsewhere in the codebase (see line 116 in useProductListingProducts.ts where 'p: currentPage + 1' is used).
| page, | |
| p: page, |
| <Transition name="fade"> | ||
| <div v-if="isFilterVisible" class="self-stretch pt-6"> |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transition should include proper ARIA attributes for screen readers. Consider adding aria-expanded to the button and aria-hidden to the transition content to improve accessibility for users with disabilities.
| <Transition name="fade"> | ||
| <div v-if="isFilterVisible" class="self-stretch flex flex-col justify-start items-start gap-4"> |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transition should include proper ARIA attributes for screen readers. Consider adding aria-expanded to the button and aria-hidden to the transition content to improve accessibility for users with disabilities.
| <Transition name="fade"> | ||
| <div | ||
| v-if="isFilterVisible" | ||
| :id="props.filter.code" | ||
| class="self-stretch flex flex-col justify-start items-start gap-4" | ||
| > |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transition should include proper ARIA attributes for screen readers. Consider adding aria-expanded to the button and aria-hidden to the transition content to improve accessibility for users with disabilities.
| <Transition name="fade"> | ||
| <div v-if="isFilterVisible" :id="props.filter.code" class="self-stretch flex flex-col justify-start items-start gap-2.5"> |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transition should include proper ARIA attributes for screen readers. Consider adding aria-expanded to the button and aria-hidden to the transition content to improve accessibility for users with disabilities.
| for (const slot of block.slots) { | ||
| if (slot.type === "product-listing") { | ||
| const listing = slot.data?.listing; | ||
| if (listing) { | ||
| return listing as T; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (const slot of block.slots) { | |
| if (slot.type === "product-listing") { | |
| const listing = slot.data?.listing; | |
| if (listing) { | |
| return listing as T; | |
| } | |
| } | |
| } | |
| const listing = block.slots | |
| .find(slot => slot.type === "product-listing") | |
| ?.data?.listing as T | undefined; | |
| if (listing) { | |
| return listing; | |
| } |
| sections: Array<{ | ||
| blocks?: Array<{ | ||
| slots?: Array<{ | ||
| type?: string; | ||
| data?: Record<string, unknown> | null; | ||
| }>; | ||
| }>; | ||
| }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| sections: Array<{ | |
| blocks?: Array<{ | |
| slots?: Array<{ | |
| type?: string; | |
| data?: Record<string, unknown> | null; | |
| }>; | |
| }>; | |
| }>; | |
| sections: { | |
| blocks?: { | |
| slots?: { | |
| type?: string; | |
| data?: Record<string, unknown> | null; | |
| }[]; | |
| }[]; | |
| }[]; |
Maybe this way will be more readable?
| order: string, | ||
| query?: operations["searchPage post /search"]["body"], | ||
| ) => { | ||
| await core.search( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion to not use API calls inside the composable. To make them more flexible and not dependent on the API client
| initialListing: Schemas["ProductListingResult"], | ||
| ): Promise<void>; | ||
| /** | ||
| * @deprecated - use `search` instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New composable and deprecation at the beginning :D?
Refactor useListing composable for better modularity and SSR support
closes: #841
blocked by: #2030
Areas covered:
Overview
Refactored the monolithic
useListingcomposable into smaller, focused modules with improved SSR support and early data access for product listings (to avoid ClientOnly loading without hydration mismatch).Changes
1. Modular Composable Architecture
Split
useListing.tsinto focused, single-responsibility composables:useListingCore.ts- Core state management with provide/inject patternlistingKeyuseProductListingProducts.ts- Product data and search operationssearch(),loadMore(),setInitialListing()useProductListingPagination.ts- Pagination logicchangeCurrentPage()useProductListingSorting.ts- Sorting functionalitychangeCurrentSortingOrder()useProductListingFilters.ts- Filter managementsetCurrentFilters(),resetFilters(),filtersToQuery()utils.ts- Shared types and utilitiesmerge(),isObject()helpers2. SSR Optimization
Early data access pattern:
getProductListingFromCmsPage.tshelper in@shopware/helpersnullif no listing foundCmsPage.vue- Extract listing on component setuplisting-related composables dependency graph
graph TD A[CmsPage.vue] -->|1. Extract listing from CMS| B[getProductListingFromCmsPage helper] B -->|2. Returns ProductListingResult or null| A A -->|3. Pass initialListing| C[createCategoryListingContext] C -->|4. Creates context with initialListing param| D[useListing composable] D -->|5. Pass initialListing| E[useListingCore] E -->|Provides shared state via inject/provide| F[Core State Management] F -.->|Injects core| G[useProductListingProducts] F -.->|Injects core| H[useProductListingPagination] F -.->|Injects core| I[useProductListingSorting] F -.->|Injects core| J[useProductListingFilters] D -->|6. Composes all modules| K[Facade Pattern useListing] K -->|Returns unified API| L[Components] L -->|Use listing composables| M[SwProductListingFilters.vue] L -->|Use listing composables| N[CmsElementProductListing.vue] L -->|Use listing composables| O[Search.vue] O -->|Manual setInitialListing call| G G -->|products, loading, search, loadMore| L H -->|pagination methods| L I -->|sorting methods| L J -->|filters methods| L P[utils.ts] -.->|Shared types & helpers| D P -.->|merge, isObject| E Q[Filter Components] -->|Use filter data| M Q1[SwFilterProperties.vue] -.-> Q Q2[SwFilterRating.vue] -.-> Q Q3[SwFilterPrice.vue] -.-> Q Q4[SwFilterShippingFree.vue] -.-> Q style A fill:#e1f5ff style E fill:#fff3e0 style D fill:#f3e5f5 style K fill:#e8f5e9 style L fill:#fce4ec subgraph "SSR Flow (Category Pages)" A B C end subgraph "Core State Management" E F end subgraph "Modular Composables" G H I J end subgraph "UI Components" M N Q end subgraph "Manual Flow (Search)" O end