-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Collection filtering UX polish #356
Conversation
…ction-filtering-ux
…ify/dawn into fix-collection-filtering-ux
…ction-filtering-ux
…ction-filtering-ux # Conflicts: # sections/main-collection-product-grid.liquid
…ction-filtering-ux # Conflicts: # locales/cs.schema.json # locales/da.schema.json # locales/de.schema.json # locales/en.default.schema.json # locales/es.schema.json # locales/fi.schema.json # locales/fr.schema.json # locales/it.schema.json # locales/ja.schema.json # locales/ko.schema.json # locales/nb.schema.json # locales/nl.schema.json # locales/pl.schema.json # locales/pt-BR.schema.json # locales/pt-PT.schema.json # locales/sv.schema.json # locales/th.schema.json # locales/tr.schema.json # locales/vi.schema.json # locales/zh-CN.schema.json # locales/zh-TW.schema.json
…ction-filtering-ux # Conflicts: # sections/main-collection-product-grid.liquid
…Shopify/dawn into fix-collection-filtering-polish
…olish" This reverts commit f59d06d.
@@ -182,7 +182,7 @@ | |||
{% endif %} |
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
@Oliviammarcello Thanks for the review. Updates have been made and are ready for another review.
Padding has been restored |
One last note about the spacing and then this is good to ship!
Note: this doesn't apply to the collection page when an image is added and the grey box appears. The current screens are perfect. Example. Thanks so much! |
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 good on my end. Works in no js as well!
<facet-remove class="mobile-facets__clear-wrapper"> | ||
<a href="{{ collection.url }}" class="mobile-facets__clear underlined-link">{{ 'sections.collection_template.clear' | t }}</a> | ||
</facet-remove> | ||
<button type="button" class="no-js-hidden button button--primary" onclick="this.closest('.mobile-facets__wrapper').querySelector('summary').click()">{{ 'sections.collection_template.apply' | t }}</button> |
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 is the first time I see a more "elaborated" inline JS code so I'm wondering if it's okay to leave it in Liquid?
Other instances throughout the theme use a more straightforward approach such as onclick="this.select();"
or onclick="window.print();"
which I understand because it's not affecting other components.
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.
Good point. I'm okay to leave this as-is for now, because this isn't actually part of the design polish and does not conflict with our dev principles. I do think this approach should be revisited to ensure:
- Adequate error handling
- Maintainability (if the target element markup is updated in the future)
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 good and works well 👍
Left just a couple comments.
assets/template-collection.css
Outdated
|
||
span.active-facets__button-inner { | ||
padding-bottom: 0.4rem; | ||
padding-top: 0.2rem; |
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.
Updated efaffd3
Oh and also I noticed a bit of a flash happening when closing the filter drawer on mobile: video but not sure what's happening. |
On mobile, I've adjusted the top margin to
On desktop, the
The filters isn't aware if the collection banner above it includes an image or not. Therefore the top margin above filters is now also |
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.
LGTM.
The one thing I notice on iOS is this focus style getting applied on the summary but not sure if it should be fixed here or in a follow up:
iosFocusFilter.MP4
Good catch, I think it would be better to apply Added fix here: defab2e |
* 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 * Add design polish to UX elements * Add design polish to UX elements * Add design polish to UX elements * Add design polish to UX elements * Add design polish to UX elements * Adjust position of loading spinner on collections page * Add design polish to UX elements * Revert dbe6059 * Revert 93eb5c9 * Fix json spacing * Change price filter input inputmode * Fix mobile filters for no-js * Address design feedback * Address design feedback * Update from Shopify for theme dawn/fix-collection-filtering-polish * Remove alignment CSS * Revert "Update from Shopify for theme dawn/fix-collection-filtering-polish" This reverts commit f59d06d. * Adjust padding of active filter * Fix filter drawer closing animation * Adjust section margins for collection banner and grid Co-authored-by: shopify-online-store[bot] <79544226+shopify-online-store[bot]@users.noreply.github.com> Co-authored-by: translation-platform[bot] <35075727+translation-platform@users.noreply.github.com>
Why are these changes introduced?
Fixes #100
What approach did you take?
Other considerations
n/a
Demo links
Checklist