You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is the first time I'm using Dawn theme. I need to modify it heavily.
I was happy to see that Dawn uses Web Components!
Seems like there are many different teams and developers working on Dawn as Shopify has grown really big recently.
It would be really useful to establish certain software development patterns so that they are followed throughout the codebase.
Describe the expected behavior
For example, I've already mentioned in another issue that Dawn uses pub/sub pattern, but only in a handful of places. Would be great if this was used more or if this wasn't used at all as there are other ways to achieve the same / similar functionality.
One issue that I hit today and spent about an hour debugging is that my HeaderDrawer component drawer wasn't being properly closed.
There have been many issues with this functionality: #2498 #2472 #1242 #1171
I believe that every drawer, popover, accordion, etc. should have "open" and "close" methods that open/close the element no matter what and that contain all the logic for properly opening/closing given element.
I looked into MenuDrawer and HeaderDrawer code and, unfortunately, this is not the case:
openMenuDrawer(summaryElement) requires summary element to be passed-in. I don't think that some other object, calling openMenuDrawer from the "outside" of MenuDrawer class, should have to pass summaryElement in order to open the drawer.
Even worse, closeMenuDrawer(event, elementToFocus = false) requires event element to be passed in as otherwise the function will immediately return.
It kinda makes sense, but perhaps we shouldn't return. We could wrap elementToFocus.classList.remove(drawer-open); in an if-statement.
We could wrap/rename/refactor closeMenuDrawer in both MenuDrawer and HeaderDrawer into onCloseMenuDrawer (since they both require an event, hence the on prefix), which calls closeMenuDrawer which always closes the drawer and can be called programmatically like this: menuDrawer.closeMenuDrawer() 😍
BTW This is a bug, this line of code does nothing as closeMenuDrawer(), when called with no arguments, returns immediately.
This whole function does nothing. It can either be removed or someone should figure out why it was needed in the first place.
It seems to have been added by "Chris Berthe" in the initial git commit. So it must be somewhere in the original/private Dawn git repo history.
The bug happens because in Theme Editor, if the menu type loaded is not drawer, but merchant switches to drawer, it will load drawer menu, but it won't query again: document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => { and therefore it doesn't properly close the drawer.
Version information (Dawn, browsers and operating systems)
Dawn Version: 9.0.0
Chrome Version 108.0.5359.124
macOS Version 13.2.1
Possible solution
I'd strongly suggest document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => { to be removed/refactored ASAP. I suggest creating Details or ExpandableDetails component and then MenuDrawer could extend it.
I'd also suggest moving any non globally needed function definitions inside an anonymous function or perhaps global Shopify or Dawn object. Currently all functions defined at the root of global.js (and any other .js file) are globally available and can conflict if apps or anything else adding functions to global scope.
Additional context/screenshots
The text was updated successfully, but these errors were encountered:
Describe the current behavior
This is the first time I'm using Dawn theme. I need to modify it heavily.
I was happy to see that Dawn uses Web Components!
Seems like there are many different teams and developers working on Dawn as Shopify has grown really big recently.
It would be really useful to establish certain software development patterns so that they are followed throughout the codebase.
Describe the expected behavior
For example, I've already mentioned in another issue that Dawn uses pub/sub pattern, but only in a handful of places. Would be great if this was used more or if this wasn't used at all as there are other ways to achieve the same / similar functionality.
One issue that I hit today and spent about an hour debugging is that my
HeaderDrawer
component drawer wasn't being properly closed.There have been many issues with this functionality:
#2498
#2472
#1242
#1171
I believe that every drawer, popover, accordion, etc. should have "open" and "close" methods that open/close the element no matter what and that contain all the logic for properly opening/closing given element.
I looked into
MenuDrawer
andHeaderDrawer
code and, unfortunately, this is not the case:openMenuDrawer(summaryElement)
requires summary element to be passed-in. I don't think that some other object, callingopenMenuDrawer
from the "outside" ofMenuDrawer
class, should have to passsummaryElement
in order to open the drawer.Even worse,
closeMenuDrawer(event, elementToFocus = false)
requiresevent
element to be passed in as otherwise the function will immediately return.Here's a relevant bit from
MenuDrawer
:I wanted to add "X" close icon and this is how I had to add it:
This doesn't seem like the way to go.
This doesn't work:
Why? Because of this line:
https://github.com/Shopify/dawn/blob/main/assets/global.js#L498
It was added in this commit by @eugenekasimov:
60b86d3
It kinda makes sense, but perhaps we shouldn't return. We could wrap
elementToFocus.classList.remove(
drawer-open);
in an if-statement.We could wrap/rename/refactor
closeMenuDrawer
in bothMenuDrawer
andHeaderDrawer
intoonCloseMenuDrawer
(since they both require an event, hence theon
prefix), which callscloseMenuDrawer
which always closes the drawer and can be called programmatically like this:menuDrawer.closeMenuDrawer()
😍It would allow us to call it exactly like this:
https://github.com/Shopify/dawn/blob/main/assets/global.js#L428
BTW This is a bug, this line of code does nothing as
closeMenuDrawer()
, when called with no arguments, returns immediately.This whole function does nothing. It can either be removed or someone should figure out why it was needed in the first place.
It seems to have been added by "Chris Berthe" in the initial git commit. So it must be somewhere in the original/private Dawn git repo history.
Why does this bug happen:
#2498
I'd bet it's because we do opening/closing logic outside of drawer classes here:
https://github.com/Shopify/dawn/blob/main/assets/global.js#L9
The bug happens because in Theme Editor, if the menu type loaded is not drawer, but merchant switches to drawer, it will load drawer menu, but it won't query again:
document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => {
and therefore it doesn't properly close the drawer.Here's the bug I'm facing, reproduced on Dawn 9.0.0 installed from the Theme Store (every time the drawer closes when I'm not clicking, it's closing because I press the ESC key - that's where the bug happens, drawer doesn't close properly):
https://user-images.githubusercontent.com/67437/232163253-7707ab9b-83f0-45f1-865d-72895d42e8dc.mov
Version information (Dawn, browsers and operating systems)
Possible solution
I'd strongly suggest
document.querySelectorAll('[id^="Details-"] summary').forEach((summary) => {
to be removed/refactored ASAP. I suggest creatingDetails
orExpandableDetails
component and thenMenuDrawer
could extend it.I'd also suggest moving any non globally needed function definitions inside an anonymous function or perhaps global
Shopify
orDawn
object. Currently all functions defined at the root ofglobal.js
(and any other.js
file) are globally available and can conflict if apps or anything else adding functions to global scope.Additional context/screenshots
The text was updated successfully, but these errors were encountered: