search

forumone / gesso

Gesso Drupal theme
66 stars 13 forks source link

Fix cloneBlock() in mobile menu script #844

Closed dcmouyard closed 1 month ago

kmonahan commented 3 months ago

@dcmouyard Can you provide some details about what wasn't working before? What errors were you getting?

dcmouyard commented 2 months ago

@kmonahan On sites that have an element (such as a search block) with an id or for attribute that we’re copying into the mobile menu, I was getting an error on e.setAttribute() due to that function not being available. This was due to how we were setting childrenWithFor, where e would be arrays instead of elements.

kmonahan commented 2 months ago

@dcmouyard Interesting. Using a NodeList vs an Array doesn't seem like it would make a difference there -- forEach still loops over each element. I'll see if I can repo.

Are the steps to reproduce:

  1. Add a search block element with an id attribute
  2. Check the console and expect to see an error on setAttribute()?
dcmouyard commented 2 months ago

@kmonahan Steps to reproduce:

  1. Make sure search block is output on the page.
  2. In source/03-components/mobile-menu/modules/_MobileMenu.es6.js update line 32 to match class of this search block. (e.g., searchBlockClass = '.c-block--id-search-form-block',)
  3. Compile theme and clear Drupal cache so that JS changes are used in Drupal.
  4. Look in the console.

I get the following error without the fix in this PR:

Uncaught TypeError: e.getAttribute is not a function
    cloneBlock _MobileMenu.es6.js:88
    cloneBlock _MobileMenu.es6.js:87
    init _MobileMenu.es6.js:332
    attach dropdown-menu.es6.ts:17
    attach dropdown-menu.es6.ts:11
    attachBehaviors drupal.js:166
    attachBehaviors drupal.js:162
    <anonymous> big_pipe.js:138
    <anonymous> big_pipe.js:169
[_MobileMenu.es6.js:88:24](webpack://gesso/source/03-components/mobile-menu/modules/_MobileMenu.es6.js)
    cloneBlock _MobileMenu.es6.js:88
    forEach self-hosted:157
    cloneBlock _MobileMenu.es6.js:87
    init _MobileMenu.es6.js:332
    attach dropdown-menu.es6.ts:17
    forEach self-hosted:157
    attach dropdown-menu.es6.ts:11
    attachBehaviors drupal.js:166
    forEach self-hosted:157
    attachBehaviors drupal.js:162
    <anonymous> big_pipe.js:138
    <anonymous> big_pipe.js:169
kmonahan commented 2 months ago

If someone else gets a chance to test this, I'm fine with it being merged in advance of my review. My desire to understand what's going on here doesn't need to be a blocker.

kmonahan commented 2 months ago

Noting that this just came up for John B. in Slack, who confirmed this fix worked.