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.

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

Duplicated scrollbar in drawer menu #2400

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Conversation

eugenekasimov
Copy link
Contributor

@eugenekasimov eugenekasimov commented Mar 14, 2023

PR Summary:

This PR is to fix duplicated scrollbar in drawer menu.

Why are these changes introduced?

Fixes #2398 .

What approach did you take?

I see there might be different approaches to fix the issue, however the most simple one for me is to add position: relative to .menu-drawer__navigation-container. I couldn't find a case it would affect anything.

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

Testing steps/scenarios

  • Open drawer on desktop or mobile and zoom in 200-250% and make sure there is only one scrolling bar
  • Make sure the drawer and language/currency forms work correct.

Demo links

Checklist

@@ -93,7 +93,6 @@ details[open].menu-opening > .menu-drawer__submenu {
display: grid;
grid-template-rows: 1fr auto;
align-content: space-between;
overflow-y: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

That works. There is another option we could choose which is applying an overflow auto on the menu nav specifically: video

Copy link
Contributor

@metamoni metamoni left a comment

Choose a reason for hiding this comment

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

🎩ed on Chrome, Safari, Firefox, Edge. Works fine for me! 👍

Copy link
Contributor

@stufreen stufreen left a comment

Choose a reason for hiding this comment

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

LGTM! Seems to be related to visually-hidden class on the h2 elements in the localization forms. I guess they rely on a parent element having position: relative otherwise this behaviour occurs.

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Works well. I wonder if we should apply the position: relative on .menu-drawer__localization instead since the issue seem to be the absolute positioning from the localization-form's h2.
This way the fix is more targeted 🤔

Ill approve anyway but just thought ^

@eugenekasimov eugenekasimov merged commit 732f076 into main Mar 15, 2023
@eugenekasimov eugenekasimov deleted the fix-double-scrollbar-drawer branch March 15, 2023 16:32
pangloss added a commit to pangloss/dawn that referenced this pull request Mar 23, 2023
* shopify/main:
  Gift cards/add recipient (Shopify#2412)
  Update 1 translation file (Shopify#2453)
  [Slideshow] Add ambient movement to background (Shopify#2383)
  Wrap calls to search results ind collection counts in paginate to reduce additional requests (Shopify#2421)
  [Header] Add app block in the header section (Shopify#2238)
  [Image with Text] Add ambient movement to image (Shopify#2385)
  Update 1 translation file (Shopify#2450)
  [Blog post section] Fix slides size on mobile  (Shopify#2368)
  Duplicated scrollbar in drawer menu (Shopify#2400)
  [Header locales] Support gradients (Shopify#2386)
  [Image banner] Add ambient movement to background (Shopify#2342)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicated scrollbar in drawer menu
4 participants