-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix collection filtering UX #268
Conversation
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.
Just leaving reminders for the auto-pushed setting updates.
@@ -1,10 +1,24 @@ | |||
{ |
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.
Reminder that we'll need to revert the changes in here
@@ -1 +1,86 @@ | |||
{} |
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.
Reminder that we'll need to revert the changes in here
Looking good so far, I have some notes:
There are a few issues with the filters that were documented here as well. |
…ction-filtering-ux
@Oliviammarcello Thanks for the review. I've addressed most issues in the list . For issues not yet addressed, I've documented them here. Here's some more details:
This was not a change introduced in this PR. To limit the scope of this PR, could we update in separate PR?
Still looking into this. @tyleralsbury any particular reason the "Filter and sort" button is hidden in CSS when the drawer is opened? details[open] .mobile-facets__open {
visibility: hidden;
}
On my end, I don't see the same misalignment, but will continue testing with different fonts
Still exploring a way to do this without adding too much code
To limit the scope of this PR, could we update in separate PR? |
@@ -17,214 +17,224 @@ | |||
<div class="page-width collection-filters" id="main-collection-filters" data-id="{{ section.id }}"> |
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.
Best to view this diff with Hide whitespace changes
enabled
@KaichenWang I'm aligned with creating separate issues if that is easier for you. Just took a look and I have a few polish notes:
|
…ify/dawn into fix-collection-filtering-ux
…ction-filtering-ux
…ction-filtering-ux # Conflicts: # sections/main-collection-product-grid.liquid
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.
Tested on all browsers, looks good.
if (containerDesktop) { | ||
containerDesktop.innerHTML = count; | ||
} |
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.
Small nitpick: does it work if we use optional chaining?
document.getElementById('CollectionProductCountDesktop')?.innerHTML = count;
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.
It's actually an interesting case - you can't use optional chaining for the left side of an assignment.
You can see the discussion on the proposal here:
.active-facets-mobile + .collection-product-count { | ||
grid-column-start: 2; | ||
grid-row-start: 2; | ||
text-align: right; | ||
} |
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.
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 .large-up-hide
is added conditionally in Liquid. Depending on the collapse_on_larger_devices
setting, the "mobile" product count may also visible on desktop:
{% unless section.settings.collapse_on_larger_devices %} medium-hide large-up-hide{% endunless %}
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.
Looks great! I tested on desktop and mobile with collapse enabled and disabled.
I noticed one thing, in the collapsible version the price still has a bubble indicating that it's being filtered. Not sure if that was intended or not, given the removal of all of the other bubbles. That was the only issue I found.
Code looks good, too. JS looks like it wasn't huge in terms of changes, and the CSS is looking nice. We might still be able to improve some of the naming for the CSS but it was already very "first draft" naming before this PR so it's a refactor we can consider later.
efd9d52
I missed that one. Now removed |
IMO, in production: english should just override the default language. Please implement that since english is understood universally in all "important" markets for most merchants. |
* Add setting to enable filtering drawer on larger devices. Adjust button styling * Update styling. Tighten spacing * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update styles and spacing for filters * Adjust font sizes for filters * Adjust alignment and spacing * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update link style for clear * Update from Shopify for theme dawn/fix-collection-filtering-ux * Address design feedback * Update locales * Update locales/cs.schema.json * Update locales/da.schema.json * Update locales/de.schema.json * Update locales/es.schema.json * Update locales/fi.schema.json * Update locales/fr.schema.json * Update locales/it.schema.json * Update locales/ja.schema.json * Update locales/ko.schema.json * Update locales/nb.schema.json * Update locales/nl.schema.json * Update locales/pl.schema.json * Update locales/pt-BR.schema.json * Update locales/pt-PT.schema.json * Update locales/sv.schema.json * Update locales/th.schema.json * Update locales/tr.schema.json * Update locales/vi.schema.json * Update locales/zh-CN.schema.json * Update locales/zh-TW.schema.json * Remove bubble for price filtering Co-authored-by: shopify-online-store[bot] <79544226+shopify-online-store[bot]@users.noreply.github.com> Co-authored-by: translation-platform[bot] <[email protected]>
Occasionally, translation strings for languages other than English may be added in a separate PR. We are working to avoid this in the future and ensure translations for all languages are included in the same PR.
This is a good suggestion. Will forward this internally to the appropriate team. |
* main: (23 commits) Cart content update (Shopify#370) Footer spacing and alignment adjustments (Shopify#369) Product Template UI polish updates (Shopify#219) footer ui updates (Shopify#318) Fix cart improvements empty state (Shopify#319) [Announcement] Adjust block id for displaying dynamic names (Shopify#327) Update translations: buyer (Shopify#329) Revert editor setting changes (Shopify#328) Update translations (Shopify#294) Fix collection filtering UX (Shopify#268) Added page width setting and fixed image quality (Shopify#292) Add top border on cart notification when "show separator line" setting is deactivated (Shopify#306) movebadge code into base.css (Shopify#313) Collage UI bug fixes (Shopify#308) Customer account UI polish (Shopify#177) Added custom liquid block to product page (Shopify#269) Fix disclosure icon (Shopify#310) Fix facet price filter label spacing (Shopify#300) Fix duplicate search icon when header logo is set to "Top center" (Shopify#252) Add card outline setting (Shopify#239) ...
* Add setting to enable filtering drawer on larger devices. Adjust button styling * Update styling. Tighten spacing * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update styles and spacing for filters * Adjust font sizes for filters * Adjust alignment and spacing * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update from Shopify for theme dawn/fix-collection-filtering-ux * Update link style for clear * Update from Shopify for theme dawn/fix-collection-filtering-ux * Address design feedback * Update locales * Update locales/cs.schema.json * Update locales/da.schema.json * Update locales/de.schema.json * Update locales/es.schema.json * Update locales/fi.schema.json * Update locales/fr.schema.json * Update locales/it.schema.json * Update locales/ja.schema.json * Update locales/ko.schema.json * Update locales/nb.schema.json * Update locales/nl.schema.json * Update locales/pl.schema.json * Update locales/pt-BR.schema.json * Update locales/pt-PT.schema.json * Update locales/sv.schema.json * Update locales/th.schema.json * Update locales/tr.schema.json * Update locales/vi.schema.json * Update locales/zh-CN.schema.json * Update locales/zh-TW.schema.json * Remove bubble for price filtering Co-authored-by: shopify-online-store[bot] <79544226+shopify-online-store[bot]@users.noreply.github.com> Co-authored-by: translation-platform[bot] <[email protected]>
Why are these changes introduced?
Fixes #198
What approach did you take?
Other considerations
TODO:Adjust no-JS implementationDemo links
Checklist