-
Notifications
You must be signed in to change notification settings - Fork 78
feat(cms-base-layer): horizontal product listing filters #2223
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
|
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
This PR adds a horizontal filter layout for product listings, introducing a new display mode when the sidebar filter element is placed outside a sidebar section. The implementation uses Vue's provide/inject pattern to detect section context and conditionally render either vertical accordion filters or horizontal dropdown filters.
Changes:
- Added two new components (
SwFilterDropdownandSwProductListingFiltersHorizontal) for horizontal filter display - Enhanced all existing filter components with a
displayModeprop to support both accordion and dropdown layouts - Implemented context awareness using provide/inject pattern in section components
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| CmsSectionSidebar.vue | Provides "sidebar" layout context to descendant components |
| CmsSectionDefault.vue | Removes overflow-auto class from blocks |
| CmsElementSidebarFilter.vue | Conditionally renders vertical or horizontal filters based on section context |
| SwProductListingFiltersHorizontal.vue | New component implementing horizontal dropdown filter layout |
| SwFilterDropdown.vue | New reusable dropdown wrapper component for horizontal filters |
| SwProductListingFilter.vue | Adds displayMode prop to pass through to individual filter components |
| SwFilterShippingFree.vue | Supports both accordion and dropdown display modes |
| SwFilterRating.vue | Supports both accordion and dropdown display modes |
| SwFilterProperties.vue | Supports both accordion and dropdown display modes |
| SwFilterPrice.vue | Supports both accordion and dropdown display modes |
| SwSortDropdown.vue | Updates label type to support translated labels and improves styling |
| .changeset/clear-tigers-press.md | Documents the feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v-for="cmsBlock in content.blocks" | ||
| :key="cmsBlock.id" | ||
| class="overflow-auto" | ||
| :content="cmsBlock" |
Copilot
AI
Jan 14, 2026
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.
CmsSectionDefault does not provide a layout context like CmsSectionSidebar does. This means filters in default sections will default to 'default' layout mode. Consider explicitly providing 'default' context in CmsSectionDefault to make the behavior more explicit and consistent with CmsSectionSidebar.
| "shipping-free": sidebarSelectedFilters["shipping-free"] as boolean, | ||
| rating: sidebarSelectedFilters.rating as number, | ||
| search: "", | ||
| limit: route.query.limit ? Number(route.query.limit) : 15, |
Copilot
AI
Jan 14, 2026
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 default limit value of 15 is duplicated in lines 96 and 218. Consider extracting this as a named constant at the top of the file to avoid magic numbers and ensure consistency.
| await executeSearch(); | ||
| } catch (error) { | ||
| console.error("Filter update failed:", error); |
Copilot
AI
Jan 14, 2026
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 error messages use console.error for production error handling without any user-facing feedback. Consider using a notification system (similar to how SwProductAddToCart uses pushError) to inform users when filter operations fail.
| query: query as LocationQueryRaw, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Search execution failed:", error); |
Copilot
AI
Jan 14, 2026
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 error messages use console.error for production error handling without any user-facing feedback. Consider using a notification system (similar to how SwProductAddToCart uses pushError) to inform users when filter operations fail.
| limit: route.query.limit ? Number(route.query.limit) : 15, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Sorting order change failed:", error); |
Copilot
AI
Jan 14, 2026
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 error messages use console.error for production error handling without any user-facing feedback. Consider using a notification system (similar to how SwProductAddToCart uses pushError) to inform users when filter operations fail.
| clearFilters(); | ||
| await executeSearch(); | ||
| } catch (error) { | ||
| console.error("Clear filters failed:", error); |
Copilot
AI
Jan 14, 2026
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 error messages use console.error for production error handling without any user-facing feedback. Consider using a notification system (similar to how SwProductAddToCart uses pushError) to inform users when filter operations fail.
| await changeCurrentSortingOrder(order, { | ||
| ...(route.query as unknown as operations["searchPage post /search"]["body"]), | ||
| limit: route.query.limit ? Number(route.query.limit) : 15, |
Copilot
AI
Jan 14, 2026
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.
This duplicates the same logic from line 96. Extract to a constant to maintain consistency.
| type SortOption = { | ||
| key: string; | ||
| label: string; | ||
| label: string | null; |
Copilot
AI
Jan 14, 2026
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 label property is defined as 'string | null' but it's accessed in the template at line 80 as 'sorting.translated?.label'. The type definition suggests label can be null, but the actual usage pattern expects a translated.label property instead. This mismatch between type definition and usage is confusing. Consider updating the type to better reflect the actual structure being used.
Description
closes: #2088
working example: https://frontends-starter-template-extend-git-10732e-shopware-frontends.vercel.app/Room-Sprays/?min-price=43&order=name-asc