Add drawer menu desktop #2195

eugenekasimov · 2023-01-04T21:38:12Z

PR Summary:

This PR adds a new desktop menu type "Drawer".

Key updates:

the drawer menu has a full width of the page. The left and right paddings are 32px.
the header's paddings for tablet have been decreased from 50px to 32px. The header is not perfectly aligned with a content anymore.
the closing X button, nav and log in icon have been aligned for desktop and tablet layouts.
the nav and log in icon have been aligned for mobile layout.

Why are these changes introduced?

Decision log

#	Decision	Alternatives	Rationale	Downsides
1	Reuse existing drawer-menu that is used for mobile/tablet layouts.	Create a new one.	I don't want to add the same code that already exists.	The current changes in code to reuse the existing drawer-menu may look not very clear because the initial approach doesn't seem to count it could be extended.
2	There was `margin-left: -0.75rem` for `header__heading-link` and it's applied all the time no matter where the `header__heading-link` was positioned . I made it more targeted for left positions only.	Leave it as is.	It helps avoid a minor shifting of `header__heading-link` when it's positioned in the center.	I don't see any of it.
3	I added an event listener to the window for the open drawer to update `width` and `height` in real time.	-	The size of the semi-transparent background which is present while the drawer is open is calculated based on the current `height` and `width` of the window. I had to deal with resizing a browser.	We need to listen for any changes of `height` but for the `width` we need to know when it passes a threshold of `990px` back-and-forward only. Since I couldn't find any good way to listen for `height` changes only the event listener listens for changes in both directions. But I believe that it's a very edge case itself and it should not cost a problem.

Visual impact on existing themes

Before

After

Testing steps/scenarios

Choose a new Desktop menu type - Drawer. Save the settings. Test it.
Test it with different Desktop logo positions. Each time when you set a new logo position don't forget to save settings. Also keep in mind that the Desktop logo position Top left and Middle left are supposed to look the same.
While you test previous steps try to change the width of your browser, shrinking it to tablet and mobile sizes. Make sure everything works correctly.

Demo links

Store
Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

…button for the drawer.

stufreen

I found a couple small issues and also have a few ideas/questions/suggestions/ Take them all with a grain of salt though as I'm new to the codebase! Let me know if any of them don't make sense or you want to chat about them.

sections/header.liquid

stufreen · 2023-01-11T14:48:15Z

sections/header.liquid

-            {%- endfor -%}
-          </ul>
-        </nav>
+        {%- if section.settings.menu_type_desktop != 'drawer' -%}


Non blocking: You could also do this with CSS by conditionally giving nav a class of drawer, and using a rule like

@media screen and (min-width: 990px) { .header__inline-menu:not(.drawer) { display: block; }

I'm not sure it matters much, but it would be consistent with what we do for small screens where the nav is part of the DOM but not displayed.

Thanks for other opinion. Both ways work for me but the current one seems more consistent to me within header.liquid file.

Let's see what other devs think.

assets/base.css

stufreen · 2023-01-11T14:59:35Z

assets/base.css

  .header--middle-left {
    grid-template-areas: 'heading navigation icons';
-    grid-template-columns: auto auto 1fr;
+    grid-template-columns: minmax(3.2rem, auto) auto 1fr;


Non blocking: What is the 3.2rem space? Looks like a magic number here, would it be helpful to put it in a named variable?

3.2rem is a size of <summary> element. When I move the element to the left outside of the grid I need the column in the grid to stay the same size.

I agree that we could put in a named variable, on the other hand, as I can see in this file we use named variables only for color properties and parameters we set the vales in JS. I would keep it simple as is.

stufreen · 2023-01-11T15:05:09Z

assets/base.css

  left: 0;
-  height: calc(var(--viewport-height, 100vh) - (var(--header-bottom-position, 100%)));
+  height: calc(var(--viewport-height, 100vh) - (var(--header-bottom-position, 100%)) - 1px);


I found a small issue if I change screen size from small to large while the menu drawer is open. The overlay appears partially overtop of the nav bar. Is the header bottom position set with JS or something? If so you might need to add a media query change listener.

Screen.Recording.2023-01-11.at.10.02.45.AM.mov

@eugenekasimov I thought that this was happening on the editor + fixed, let's make sure it is not happening on the store.

Good catch @stufreen, thanks. Actually I found that it's also happening from large to small screen changing but the issue appears at the bottom. I'm going to fix it.

Updated
I fixed the bug and added a description of my approach into the decision log (4).

assets/global.js

sections/header.liquid

metamoni

On mobile, it looks like some styles are not applied properly. Some of the menu items remain highlighted and it's confusing, as I don't know if they're highlighted because they're visited links, or because they're selected.

Never mind, I see this is happening on main too

drawer.menu.mobile.mp4

stufreen

LGTM with a couple small changes made ✨

stufreen · 2023-01-12T20:30:41Z

assets/global.js

+    this.onResize = () => {
+      document.documentElement.style.setProperty('--header-bottom-position', `${parseInt(this.header.getBoundingClientRect().bottom - this.borderOffset)}px`);
+      document.documentElement.style.setProperty('--viewport-height', `${window.innerHeight}px`);
+    };


I can see that what we did yesterday didn't work (onResize() {) because this wasn't bound. However I think it's cleaner to use a class method with an arrow function, which is also valid and will automatically bind this:

onResize = () => { ... }

Yeah, and I even tried .bind(this) and it didn't work. But I didn't try to use an arrow function instead of a normal one. Now this works and looks better I agree. Thanks.

Always using arrow functions prevents these kinds of bugs. You don't have to think whether you need to .bind(this) or not and if you need to omit .bind(this) so that this refers to something else, you're probably writing a code smell (IMO). In that case, it'd be better to move function outside of class. All on* functions should be arrow functions. For the sake of consistency, you could have all JS class functions be arrow functions. There's probably ESLint rule that you can enable for this.

EDIT: I didn't find the ESLint rule. Maybe there's one, I just didn't spend too much time looking into this.

stufreen · 2023-01-12T20:34:15Z

assets/global.js

@@ -430,13 +435,18 @@ class HeaderDrawer extends MenuDrawer {
    });

    summaryElement.setAttribute('aria-expanded', true);
+    window.addEventListener('resize', this.onResize);


Just summarizing a conversation we had yesterday:

resize listener can be a bit heavy since it's called repeatedly when you resize a window; however

it's appropriate here because the overlay needs to be resized along with the browser. There's no getting around redrawing the overlay each time the browser is resized vertically.

As you pointed out, there's a bug in production where the overlay gets cut off if the user enlarges the browser window vertically. This will fix it.

assets/global.js

stufreen · 2023-02-21T19:28:21Z

assets/component-menu-drawer.css

+    display: none;
+  }
+
+  .menu-drawer__utility-links:has(ul:empty) {


I guess the alternative would be to do something like @LucasLacerdaUX did here where we check in Liquid if the social icons are set and if so omit the whole area: #2226

Personally I am OK with a progressive enhancement here. It will work for almost all buyers and not look terribly broken for the rest.

assets/base.css

sections/header.liquid

ludoboludo

Just setting it to request changes so we can fix some of the issues I found

assets/component-cart-notification.css

sections/header.liquid

snippets/cart-notification.liquid

assets/base.css

ludoboludo

🚀

melissaperreault · 2023-03-03T22:33:20Z

@eugenekasimov I found a bug that I think is related to that merge. Page section has lost its width reference, becoming too wide for readable content. Video reference

Do you prefer I open a new issue and take it from there?

* shopify/main: Improve image sizes in the multicolumn section (Shopify#2349) Fix the Page section's width. (Shopify#2364) Update 12 translation files (Shopify#2366) Removing "my" from cart popup notification (Shopify#2353) [Cart.js] Fix fetch url so it's not hard coded (Shopify#2357) (Shopify#2365) Update 1 translation file (Shopify#2352) Default Follow on Shop to on [Header] Add localization selectors (Shopify#2258) Remove async CSS pattern where it may introduce layout shifts (Shopify#2270) Change rich text section heading to be of type inline_richtext, also moved rte styling into base.css (Shopify#2326) Add drawer menu desktop (Shopify#2195) Make header image preload and proper width (Shopify#2307)

* Add drawer option to the menu settings * Add logic and styles for drawer menu * Add a new breakpoint for desktop layout. Implement movement of close button for the drawer. * Change a css rule order * Minor changes in css to position close-button correctly * Minor changes in css to position close-button correctly * Minor changes in base.css and header.liquid * Fix resizing issue for open drawer * Clean up JS logic. * Remove repositioning of closing button X. Align log-in icon and nav. Decrease the header padding for tablet. * Remove unnecessary css rule. * Minor refactoring * Hide login in the drawer for tablet/desktop. * Refactoring css * Change naming for grid system * Refactor header.liquid. Remove else and add elsif. * Polish css. Add role navigation for drawer-menu. * Remove role navigation and add nav tag * Remove nav * Change order of conditional classes. Correct indentation. * Minor chnages in grid system. * Move the cart-notification according to the menu type. * Fix typo in comment. * Minor fix for middle-center logo position. * Add condition for .header-middle-center for drawer * Minor changes in css for .header__search * Refactor and polish css. * Update 20 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

eugenekasimov added 4 commits December 15, 2022 11:11

Add drawer option to the menu settings

c0fd5d5

Merge branch 'main' into add-drawer-menu-desktop

714c817

Add logic and styles for drawer menu

cad5ad8

Add a new breakpoint for desktop layout. Implement movement of close …

8897571

…button for the drawer.

github-actions bot added the cla-needed label Jan 4, 2023

Change a css rule order

2c5239b

eugenekasimov requested a review from duygukalaycioglu January 4, 2023 23:38

eugenekasimov force-pushed the add-drawer-menu-desktop branch from 15cd0d8 to 2c5239b Compare January 5, 2023 19:52

github-actions bot removed the cla-needed label Jan 5, 2023

eugenekasimov marked this pull request as ready for review January 5, 2023 22:45

eugenekasimov and others added 3 commits January 9, 2023 10:00

Merge branch 'main' into add-drawer-menu-desktop

d3e5472

Minor changes in css to position close-button correctly

4eb0e1b

Minor changes in css to position close-button correctly

900860d

duygukalaycioglu previously approved these changes Jan 11, 2023

View reviewed changes

stufreen suggested changes Jan 11, 2023

View reviewed changes

eugenekasimov added 2 commits January 11, 2023 22:12

Minor changes in base.css and header.liquid

296a84d

Fix resizing issue for open drawer

033cd80

eugenekasimov dismissed duygukalaycioglu’s stale review via 033cd80 January 12, 2023 06:14

metamoni reviewed Jan 12, 2023

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

metamoni reviewed Jan 12, 2023

View reviewed changes

sections/header.liquid Outdated Show resolved Hide resolved

metamoni reviewed Jan 12, 2023

View reviewed changes

sections/header.liquid Outdated Show resolved Hide resolved

metamoni reviewed Jan 12, 2023

View reviewed changes

eugenekasimov requested review from stufreen and metamoni January 12, 2023 19:53

stufreen previously approved these changes Jan 12, 2023

View reviewed changes

Clean up JS logic.

60b86d3

eugenekasimov dismissed stufreen’s stale review via 60b86d3 January 12, 2023 22:28

eugenekasimov requested a review from stufreen January 16, 2023 16:52

eugenekasimov changed the title ~~Add drawer menu desktop~~ Add drawer menu desktop (Don't merge) Jan 16, 2023

eugenekasimov changed the title ~~Add drawer menu desktop (Don't merge)~~ Add drawer menu desktop ( ❌ Don't merge) Jan 16, 2023

stufreen previously approved these changes Feb 21, 2023

View reviewed changes

ludoboludo reviewed Feb 21, 2023

View reviewed changes

assets/base.css Show resolved Hide resolved

assets/base.css Outdated Show resolved Hide resolved

sections/header.liquid Outdated Show resolved Hide resolved

sections/header.liquid Outdated Show resolved Hide resolved

ludoboludo requested changes Feb 21, 2023

View reviewed changes

eugenekasimov added 3 commits February 22, 2023 09:24

Change order of conditional classes. Correct indentation.

909255a

Minor chnages in grid system.

7d76e59

Move the cart-notification according to the menu type.

0e887ed

eugenekasimov dismissed stale reviews from stufreen and metamoni via 0e887ed February 23, 2023 00:18

eugenekasimov added 4 commits February 22, 2023 16:24

Fix typo in comment.

87439fd

Minor fix for middle-center logo position.

29f883c

Add condition for .header-middle-center for drawer

1138f0a

Minor changes in css for .header__search

7ddb22b

eugenekasimov requested a review from ludoboludo February 23, 2023 07:20

ludoboludo reviewed Feb 23, 2023

View reviewed changes

assets/component-cart-notification.css Show resolved Hide resolved

sections/header.liquid Outdated Show resolved Hide resolved

snippets/cart-notification.liquid Outdated Show resolved Hide resolved

assets/base.css Outdated Show resolved Hide resolved

ludoboludo reviewed Feb 23, 2023

View reviewed changes

Refactor and polish css.

ae042a3

eugenekasimov force-pushed the add-drawer-menu-desktop branch from 848ef5a to ae042a3 Compare February 24, 2023 21:55

eugenekasimov and others added 2 commits February 24, 2023 14:33

Merge branch 'main' into add-drawer-menu-desktop

80fb403

Update 20 translation files

865b415

eugenekasimov requested a review from ludoboludo February 24, 2023 23:11

melissaperreault self-requested a review February 27, 2023 15:58

melissaperreault approved these changes Feb 27, 2023

View reviewed changes

metamoni approved these changes Feb 27, 2023

View reviewed changes

ludoboludo approved these changes Feb 28, 2023

View reviewed changes

eugenekasimov merged commit d75eeb8 into main Feb 28, 2023

eugenekasimov deleted the add-drawer-menu-desktop branch February 28, 2023 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add drawer menu desktop #2195

Add drawer menu desktop #2195

eugenekasimov commented Jan 4, 2023 •

edited

Loading

stufreen left a comment

stufreen Jan 11, 2023

eugenekasimov Jan 11, 2023

stufreen Jan 11, 2023

eugenekasimov Jan 12, 2023

stufreen Jan 11, 2023

duygukalaycioglu Jan 11, 2023

eugenekasimov Jan 11, 2023 •

edited

Loading

metamoni left a comment •

edited

Loading

stufreen left a comment

stufreen Jan 12, 2023

eugenekasimov Jan 12, 2023 •

edited

Loading

vfonic Feb 10, 2023 •

edited

Loading

stufreen Jan 12, 2023 •

edited

Loading

stufreen Feb 21, 2023

ludoboludo left a comment

ludoboludo left a comment

melissaperreault commented Mar 3, 2023

Add drawer menu desktop #2195

Add drawer menu desktop #2195

Conversation

eugenekasimov commented Jan 4, 2023 • edited Loading

PR Summary:

Why are these changes introduced?

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

stufreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenekasimov Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

metamoni left a comment • edited Loading

Choose a reason for hiding this comment

stufreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenekasimov Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

vfonic Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

stufreen Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

melissaperreault commented Mar 3, 2023

eugenekasimov commented Jan 4, 2023 •

edited

Loading

eugenekasimov Jan 11, 2023 •

edited

Loading

metamoni left a comment •

edited

Loading

eugenekasimov Jan 12, 2023 •

edited

Loading

vfonic Feb 10, 2023 •

edited

Loading

stufreen Jan 12, 2023 •

edited

Loading