Fixed active facets tap target and drawer scrolling #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sofiamatulis
reviewed
Jul 5, 2021
| <span class="active-facets__button-inner button button--tertiary"> | ||
| {%- if filter.min_value.value -%}{{ filter.min_value.value | money }}{%- else -%}{{ 0 | money }}{%- endif -%}-{%- if filter.max_value.value -%}{{ filter.max_value.value | money }}{%- else -%}{{ filter.range_max | money }}{%- endif -%} | ||
| {% render 'icon-close-small' %} | ||
| </span> | ||
| </a> | ||
| {%- endif -%} |
There was a problem hiding this comment.
On Safari, the background is still scrollable: https://screenshot.click/05-51-wiacu-ueec2.mp4
There was a problem hiding this comment.
That's weird. Version? It seems to be working as far as I can tell.
There was a problem hiding this comment.
I haven't been able to reproduce it 🤔
There was a problem hiding this comment.
Can't replicate anymore on my iPad either. 👍
|
Looks good @tyleralsbury. Just have a few notes related to the active facets tap targets; drawer scrolling looks great. FYI All other UI notes have just been added to a polish review here.
|
ludoboludo
reviewed
Jul 5, 2021
|
Should be ok for a second round.
|
sofiamatulis
approved these changes
Jul 7, 2021
ludoboludo
approved these changes
Jul 7, 2021
Oliviammarcello
approved these changes
Jul 7, 2021
|
Merged - thanks for the review, y'all! |
stufreen
pushed a commit
that referenced
this pull request
Mar 15, 2023
* Remove default section in index and product templates for experiment 1 * Add 4th digit to theme version number * Remove release notes * Remove footer blocks * Revert "Remove release notes" This reverts commit ec430c31ef5a4738059dd00036b29992aa4691b6. * Revert "Add 4th digit to theme version number" This reverts commit a301e58a6e1d8ce0a090d0bba460f02b58c29e8f. * Fix whitespace
phapsidesGT
pushed a commit
to Gravytrain-UK/gt-shopify-dawn-theme
that referenced
this pull request
Sep 3, 2024
* Filtering - Fixed a11y issue with tap targets on active filters * Added drawer functionality to handle overflow changes at different breakpoints
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes introduced?
First part of faceted filtering fast follow fixes. #100
What approach did you take?
Tap targets
Changed up the markup a bit for the pills so that they now have a wrapper
<a>with a<span>inside that have the pill styles. The<a>has padding to ensure that it will always be at least44px(actually45pxgiven the current line heights, etc...) to follow accessible tap target principlesSource - w3.org
Scrolling issue
When a drawer is open, the content behind should not be scrollable. The
menu-drawercustom element was already adding a class for this, but the class wasn't doing anything. I addedoverflow-y: hiddento prevent vertical scrolling when the class is on the body.Other considerations
The
menu-drawercustom element is also used for the main menu and the overflow breakpoints are different for the two. I've needed to add a mobile version and a tablet version of the overflow hidden class.Testing
Store
Checklist