Skip to content
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.

Sign up for GitHub

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

Prevent scrollbar overflow in footer disclosures #1293

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Prevent scrollbar overflow in footer disclosures #1293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d79e89
Prevent scrollbar overflow in footer disclosures
kmeleta Jan 31, 2022
d7bd25d
Add scroll padding
kmeleta Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions assets/disclosure.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,11 @@
background-color: transparent;
}

.disclosure__list {
.disclosure__list-wrapper {
border-width: var(--popup-border-width);
border-style: solid;
border-color: rgba(var(--color-foreground), var(--popup-border-opacity));
font-size: 1.4rem;
margin-top: -0.5rem;
min-height: 8.2rem;
max-height: 19rem;
max-width: 22rem;
min-width: 12rem;
width: max-content;
overflow-y: auto;
padding-bottom: 0.5rem;
padding-top: 0.5rem;
overflow: hidden;
position: absolute;
bottom: 100%;
transform: translateY(-1rem);
Expand All @@ -35,6 +26,20 @@
box-shadow: var(--popup-shadow-horizontal-offset) var(--popup-shadow-vertical-offset) var(--popup-shadow-blur-radius) rgba(var(--color-shadow), var(--popup-shadow-opacity));
}

.disclosure__list {
position: relative;
overflow-y: auto;
font-size: 1.4rem;
padding-bottom: 0.5rem;
padding-top: 0.5rem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also an issue on cart note:

https://screenshot.click/10-33-0n390-e7xaj.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering the text area issue separately because it's more complex.

The same happens to the cart note I believe but since it's going to need a different approach it most likely should be a different issue/PR. It's also kind of an edge case since it only comes up if you write a lot of info in the cart note: screenshot

Hmm yeah I remember that, guess we never logged that in the same issue. But I just did a quick audit and it's also going to be an issue with the textarea on blogs and contact. I was gonna throw that in, but I'm not sure what approach to use for it, given an overflow: hidden would also hide the focus border animation 🤔 Let's plan to handle separately for now. cc ludoboludo

scroll-padding: 0.5rem 0;
min-height: 8.2rem;
max-height: 19rem;
max-width: 22rem;
min-width: 12rem;
width: max-content;
}

.disclosure__item {
position: relative;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we add corner radius, the scrollbar gets a bit cut off: https://screenshot.click/10-34-vbl98-jvwxu.mp4

Copy link
Contributor Author

@kmeleta kmeleta Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing we can do about that with this approach (the solution specifically aims to mask the scrollbar). The only other possible solution (that I see anyway) to is just give the scrollable list enough margin so it stays away from the corners (which is kinda ugly).

Copy link
Contributor

@melissaperreault melissaperreault Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aligned that we can live with the tradeoff of cutting it off. Merchant would have other ways to go around this like reducing the border thickness and roundness if they have long list of currency/country. Very edge case!

Expand Down
2 changes: 1 addition & 1 deletion assets/section-footer.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ noscript .localization-selector.link {
color: #000000;
}

.localization-selector + .disclosure__list {
.localization-selector + .disclosure__list-wrapper {
margin-left: 1rem;
opacity: 1;
animation: animateLocalization var(--duration-default) ease;
Expand Down
42 changes: 23 additions & 19 deletions sections/footer.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,15 +253,17 @@
{{ localization.country.name }} ({{ localization.country.currency.iso_code }} {{ localization.country.currency.symbol }})
{% render 'icon-caret' %}
</button>
<ul id="FooterCountryList" role="list" class="disclosure__list list-unstyled" hidden>
{%- for country in localization.available_countries -%}
<li class="disclosure__item" tabindex="-1">
<a class="link link--text disclosure__link caption-large{% if country.iso_code == localization.country.iso_code %} disclosure__link--active{% endif %} focus-inset" href="#"{% if country.iso_code == localization.country.iso_code %} aria-current="true"{% endif %} data-value="{{ country.iso_code }}">
{{ country.name }} <span class="localization-form__currency">({{ country.currency.iso_code }} {{ country.currency.symbol }})</span>
</a>
</li>
{%- endfor -%}
</ul>
<div class="disclosure__list-wrapper" hidden>
<ul id="FooterCountryList" role="list" class="disclosure__list list-unstyled">
{%- for country in localization.available_countries -%}
<li class="disclosure__item" tabindex="-1">
<a class="link link--text disclosure__link caption-large{% if country.iso_code == localization.country.iso_code %} disclosure__link--active{% endif %} focus-inset" href="#"{% if country.iso_code == localization.country.iso_code %} aria-current="true"{% endif %} data-value="{{ country.iso_code }}">
{{ country.name }} <span class="localization-form__currency">({{ country.currency.iso_code }} {{ country.currency.symbol }})</span>
</a>
</li>
{%- endfor -%}
</ul>
</div>
</div>
<input type="hidden" name="country_code" value="{{ localization.country.iso_code }}">
</div>
Expand Down Expand Up @@ -296,15 +298,17 @@
{{ localization.language.endonym_name | capitalize }}
{% render 'icon-caret' %}
</button>
<ul id="FooterLanguageList" role="list" class="disclosure__list list-unstyled" hidden>
{%- for language in localization.available_languages -%}
<li class="disclosure__item" tabindex="-1">
<a class="link link--text disclosure__link caption-large{% if language.iso_code == localization.language.iso_code %} disclosure__link--active{% endif %} focus-inset" href="#" hreflang="{{ language.iso_code }}" lang="{{ language.iso_code }}"{% if language.iso_code == localization.language.iso_code %} aria-current="true"{% endif %} data-value="{{ language.iso_code }}">
{{ language.endonym_name | capitalize }}
</a>
</li>
{%- endfor -%}
</ul>
<div class="disclosure__list-wrapper" hidden>
<ul id="FooterLanguageList" role="list" class="disclosure__list list-unstyled">
{%- for language in localization.available_languages -%}
<li class="disclosure__item" tabindex="-1">
<a class="link link--text disclosure__link caption-large{% if language.iso_code == localization.language.iso_code %} disclosure__link--active{% endif %} focus-inset" href="#" hreflang="{{ language.iso_code }}" lang="{{ language.iso_code }}"{% if language.iso_code == localization.language.iso_code %} aria-current="true"{% endif %} data-value="{{ language.iso_code }}">
{{ language.endonym_name | capitalize }}
</a>
</li>
{%- endfor -%}
</ul>
</div>
</div>
<input type="hidden" name="locale_code" value="{{ localization.language.iso_code }}">
</div>
Expand Down Expand Up @@ -341,7 +345,7 @@
this.elements = {
input: this.querySelector('input[name="locale_code"], input[name="country_code"]'),
button: this.querySelector('button'),
panel: this.querySelector('ul'),
panel: this.querySelector('.disclosure__list-wrapper'),
};
this.elements.button.addEventListener('click', this.openSelector.bind(this));
this.elements.button.addEventListener('focusout', this.closeSelector.bind(this));
Expand Down