Help needed understanding statements in function


#23

Your right, the opacity disguised the curtain effect of clip-path for me.
I wasn't aware of the clip-path method, I'll take a closer look at it.

Yes that extra height needed for insurance is what I called "lag time", it is not very pleasant.

The magic number trap is why I called it a last-resort.

Sounds like clip-path may work out, I'll look into later. I'm on my way out the door right now.

Thanks Paul


#24

Just in case you missed it I must re-iterate that IE/Edge doesn't support it yet so if that's an issue then you may have to revert back to height. These days IE/Edge usage is pretty low so as long as they get something useful I tend not to bother and Edge will likely add support anyway.


#25

I was able to resolve that by removing the .expand and .open classes that were added to the ul and button.

Two new lines of .classList.remove in the matchMedia function removes the classes and resolves the confusion of which state it was in.

Hopefully that resolves the error in my logic. :slight_smile:

As seen in the updated page.

   var screenTest = function(e) {
      if (e.matches) {
         console.log('Viewport smaller than 651px');;
         menuNode.style.height = 0;
      }
      else {
         console.log('Viewport larger than 651px');
         menuNode.style.height = height + 'px';
         menuNode.classList.remove("expand");
         button.classList.remove("open");
      }
   }

#26

Hi Again,
I need to pick up where I left off, after Paul_Wilkins cleaned up the script in post #17.

I am now trying to merge the script in to my actual page template and I am encountering the same problems I had in the thread where the template was refined. Only now I know what the problem is, so I know how to explain it better.

Toggle Only - working in all browsers.

It's important to start at that page as it explains everything in detail. Then it links to a page where the Slide Toggle script from this thread is being merged into the page template.

I need help setting up a new ClickHandler function and I have no idea how to write it or merge it into the Slide Toggle script you guys helped me with. :worried:

Thanks for Reading!

EDIT:

To further illustrate what is being explained in my page link, here are some screenshots of FF and chrome.

Here it shows how Firefox still retains the sidebar div with display:contents;

Chrome shows how the unwrap.js script removes the sidebar div, and the need to target the clickhandler from the .inner div


#27

At the risk of butting in where I'm not wanted the issue seems to me that you should be adding the event listener on the match media once the html has been created and then it will know where it is. (I guess that js has a way of adding events to dynamically added code but that is above my pay scale :)).

In the interestes of first finding something that works then perhaps you could give this a try:

Hopefully @Paul_Wilkins will be around later to tidy it up and correct it.:wink:

I just changed this part:

function expandableMenu() {
    var mql = window.matchMedia('(max-width: 650px)');

    var screenTest = function(e) {
        var menuNode = document.querySelector('.menu');
        var handler = document.querySelector('.toggle');
        var height = menuNode.offsetHeight;
        menuNode.style.height = 0;
        if (e.matches) {

            console.log('Viewport smaller than 651px');
            menuNode.style.height = 0;
            handler.addEventListener('click', function() {
                toggleClass(menuNode, height);
            })
        } else {
            console.log('Viewport larger than 651px');
            menuNode.style.height = 'auto';
            menuNode.classList.remove("expand");
            handler.classList.remove("open");
            handler.removeEventListener('click', function() {});
        }
    }
    screenTest(mql);
    mql.addListener(screenTest);

    mql.onchange = function() {
        console.log(mql);
    }

    var toggleClass = function(menuNode, height) {
        var isExpanded = menuNode.classList.toggle("expand");
        menuNode.style.height = isExpanded ? height + "px" : 0;
    }
}


expandableMenu();

Seems to work for me but of course I could have missed a simple and obvious solution :slight_smile:


#28

Paul, your help is always appreciated!
In fact, had it not been for you I wouldn't be this far along with the page. :slight_smile:

I am trying to work from the concept that @Pullo introduced in post #38 of the unwrap thread.

That concept is what I was referring to in my Toggle Only Page, when I had said this...

Since the ClickHandler function targets everything by way of the.inner div. Then technically the child node locations in the DOM are not effected when the .sidebar div is removed by browsers that don't support display:contents. This way a page reload is not necessary when the browser window changes widths.

Below that comment in my page was the simple toggle only script that showed how it targets the .inner div with var inner, then function toggleMenuClickHandler locates the button by it's className.

function toggleMenuClickHandler(evt) {
   var clickedEl = evt.target;
      if(clickedEl.className === "toggle"||"toggle.open") {
         /* toggle the .expand class to UL.menu */
      clickedEl.nextElementSibling.classList.toggle("expand");
         /* toggle the .open class to button */
      clickedEl.classList.toggle("open");
   }
}

var inner = document.querySelector(".inner");
inner.addEventListener("click", toggleMenuClickHandler);

In doing it that way, one clickhandler takes care of all class toggling. On the menu and the button.

I've lost my hamburger animation in your example.
That's what toggling the .open class to the button was doing.

I'm just trying to roll everything together now, as explained on my Slide Toggle page.

I need help setting up a new ClickHandler function for the "Slide Toggle" script (view page source), that targets everything by way of the .inner div.

On that Slide Toggle page I had some comments in the JS, I was hoping they would give some direction to what I am trying to achieve.

Here is the new slide toggle script that @Andres_Vaquero & @Paul_Wilkins helped with. I just need to rework the clickhandler so it works from the .inner div :slight_smile:

var expandableMenu = function(menuNode, handler) {
   var height = menuNode.offsetHeight;
   menuNode.style.height = 0;
   var mql = window.matchMedia('(max-width: 650px)');

   var screenTest = function(e) {
      if (e.matches) {
         console.log('Viewport smaller than 651px');;
         menuNode.style.height = 0;
      }
      else {
         console.log('Viewport larger than 651px');
         menuNode.style.height = 'auto';
         menuNode.classList.remove("expand");
         button.classList.remove("open");
      }
   }
   screenTest(mql);
   mql.addListener(screenTest);

   mql.onchange = function() {
      console.log(mql);
   }

   var toggleClass = function(){
      var isExpanded = menuNode.classList.toggle("expand");
      menuNode.style.height = isExpanded ? height + "px" : 0;
   }
   // New clickHandler would replace this I assume
   handler.onclick = function(){
      toggleClass();
   }
}

var node = document.querySelector('.menu');
var button = document.querySelector('.toggle');
expandableMenu(node, button);

// Toggle the .open class to button for hamburger animations
// New clickHandler function below will replace or include this
button.addEventListener('click', function (){
this.classList.toggle('open');
})

/*
// The clickHandler below needs to be merged into the expandableMenu function above

var clickHandler = function(evt) {
   var clickedEl = evt.target;
      if(clickedEl.className === 'toggle'||'toggle.open') {
      // need to run 'expandableMenu function' from clickedEl.nextElementSibling
      clickedEl.nextElementSibling.----------;
      // toggle the .open class to button for hamburger animations
      clickedEl.classList.toggle("open");
   }
}

var inner = document.querySelector('.inner');
inner.addEventListener("click", clickHandler);
*/

#29

I've put that back in now as I didn't notice it :slight_smile:

Yes that's the only part I changed and looks like this in my example:

function expandableMenu() {
    var mql = window.matchMedia('(max-width: 650px)');

    var screenTest = function(e) {
        var menuNode = document.querySelector('.menu');
        var handler = document.querySelector('.toggle');
        var height = menuNode.offsetHeight;
        menuNode.style.height = 0;
        if (e.matches) {

            console.log('Viewport smaller than 651px');
            menuNode.style.height = 0;
            handler.addEventListener('click', function() {
                toggleClass(menuNode, height,handler);
            })
        } else {
            console.log('Viewport larger than 651px');
            menuNode.style.height = 'auto';
            menuNode.classList.remove("expand");
            handler.classList.remove("open");
            handler.removeEventListener('click', function() {});
        }
    }
    screenTest(mql);
    mql.addListener(screenTest);

    mql.onchange = function() {
        console.log(mql);
    }

    var toggleClass = function(menuNode, height,handler) {
        var isExpanded = menuNode.classList.toggle("expand");
        menuNode.style.height = isExpanded ? height + "px" : 0;
		handler.classList.toggle('open');
		
    }
}
expandableMenu();

You were creating a reference to the elements at the start but of course the html is changed later on so doesn't work. I made the check inside the matchmedia after the html has been changed which is why it works. I don't see why you would want to keep pulling the functionality out of the precise point where you need it?

Obviously @Paul_Wilkins can tidy up the solution but its often good to start from a point where everything is working and then customise and make more efficient:)


#30

Hi Paul,
Yes, your example now works as intended. :slight_smile:
Thank You!

I understand, that's why I kept pointing to Pullo's concept of working from .inner as a reference point for everything to work from.

I assumed it was a feasible way of targeting the button element, which fired the clickhandler, since I was learning from his example. And in the case of the simple toggle only menu it may have been the easiest way.

Right, I see that now.

Just as with us in the CSS forum, there's always "more than one way to skin a cat". But some methods are more robust than others.

That's where I was hoping for some input from the JS experts.

I'm not sure if one clickhandler working from .inner is better (or worse) than using handler.removeEventListener

From looking at your script though, it does look simpler than going through all the motions of working from .inner

If no one pitches in about a more efficient way of doing this then I will use your script.

Thanks for helping me Paul :slight_smile:


#31

I spoke too soon, now it's broken in Firefox. :worried:

Open the page in FF in desktop or mobile view.

  1. Drag browser window back and forth
  2. Slide toggle opens
  3. Drag back and forth again
  4. Page then requires a re-load to open slide toggle

Chrome seems fine though.
( For now, I suspect it would suffer the same problem when chrome ships display:contents)

I still feel like a new clickhandler needs to be set up that works it's way off the .inner div as I mentioned in post#28.

The Slide Toggle page works "as is" in Firefox using display:contents.
It serves as an example of how Chrome and IE needs to work.

That is how we ironed out the problems with the unwrap script in the other thread.
Using FF as a reference, and then provided adjustments for other browsers.


#32

OK sorry Ray I'll admit defeat and leave it someone who knows what they are doing :slight_smile:

In Firefox it seems the toggle is happening twice as the menu is being opened and closed straight away but if you open and close the window twice it works again!

Also I think the removeEventListener is not working as I believed it needs to be named.


#33

No worries Paul, I'm sure it will get sorted out.

The proof of concept for what needs to happen is already in place and working in the Toggle Only Page.

This is the exact same thing that was happening in the other thread and @Pullo has already shown how to solve the problem by working from the .inner div.

var inner = document.querySelector(".inner");
inner.addEventListener("click", expandableMenu);

For the sake of example you'll see that I renamed the function there as expandableMenu for what needs to happen.

Then these critical pieces below need to make their way into expandableMenu as they are what makes it all run from the.inner div.

var clickedEl = evt.target;
      if(clickedEl.className === "toggle"||"toggle.open") {
         /* target menuNode by way of nextElementSibling*/
      clickedEl.nextElementSibling.classList.toggle("expand");
         /* toggle the .open class to button */
      clickedEl.classList.toggle("open")

clickedEl.nextElementSibling

is my ul .menu and the and it would be the same as...

var node = document.querySelector('.menu');

But I don't know how to write all that into the new script.

I fear the more posts I make, the more the thread gets cluttered up, and harder to follow. My test pages explained everything, it's just a matter of getting some help from a JS expert.


#34

Hi @Ray.H I modified the script a bit hoping to fix your issue. I think the problem is that you're passing anonymous functions to addEventListener and removeEventListener. When you pass an anonymous function to addEventListener then you cannot remove it with removeEventListener as you do not have a reference to the function. You can fix that with the following modifications to your script (note I have not tested them as I am not able to atm but it is my best try, hope it works) :

function expandableMenu() {
    var mql = window.matchMedia('(max-width: 650px)');
    var menuNode = document.querySelector('.menu');
    var handler = document.querySelector('.toggle');
    var height = menuNode.offsetHeight;

    var toggleClass = function(menuNode, height, handler) {
        var isExpanded = menuNode.classList.toggle("expand");
        menuNode.style.height = isExpanded ? height + "px" : 0;
        handler.classList.toggle('open');
    }
    
    var onClick = function(e) {
        toggleClass(menuNode, height, handler);
    };
    
    var screenTest = function(e) {
        height = menuNode.offsetHeight;
        menuNode.style.height = 0;
        
        if (e.matches) {
            console.log('Viewport smaller than 651px');
            menuNode.style.height = 0;
            handler.addEventListener('click', onClick);
        } else {
            console.log('Viewport larger than 651px');
            menuNode.style.height = 'auto';
            menuNode.classList.remove("expand");
            handler.classList.remove("open");
            handler.removeEventListener('click', onClick);
        }
    }
    screenTest(mql);
    mql.addListener(screenTest);
    mql.onchange = function() {
        console.log(mql);
    }
}
expandableMenu();

#35

Hi Andres,
Thanks for helping!

I just uploaded this test page with your modifications to the script.

toggle-slide-2.html
menu-slide-toggle-2.js

Yes, the page works in Firefox since it uses display:contents;

But it suffers from needing the page reloaded in Chrome when the browser window changes width.
That's what I keeping referring back to, in how it gets resolved by targeting from the .inner div since changes in the DOM don't effect it.

The display:contents; portion of my page layout may be new to you. It was never part of the original slide toggle page you helped me with.

EDIT:
If you go back to post#26 and click on the Toggle Only link it will give a full explanation of the page layout using display:contents; and the unwrap.js script for browsers that don't support it. :slight_smile:


#36

Hey Ray, maybe we need to go and revisit the original solution after all. I haven't read this thread in its entirety, but if you can make a demo page demonstrating the problem and include a brief description of what is not working, then I can have a look for you.


#37

Post #26 has all the details you need and a link to a demo explaining the problem clearly :slight_smile:


#38

Hi Pullo,
I'm glad to see you drop in :slight_smile:

As Paul mentioned, all the details and demo are in post #26

The Toggle Only page is using the script you wrote in my other thread.
It is proof of concept that working from the .inner div is the way to handle changes in the DOM, for browsers that get the unwrap script.

The screenshots in post#26 will compliment everything that is said in my demo page.

What My Goal Is

I am now trying to get the Slide Toggle menu working in my flexbox & display:contents page layout

After reading the Toggle Only page, there is a link at the bottom that takes you to the Slide Toggle page.

There are links at the bottom of the Slide Toggle page so you can grab the files and work with them.That page has the slide toggle script that was developed earlier on in this thread.

Some modifications were made
The menu-slide-toggle-2.js seen in post #35 was Andres_Vaquero's refinement of PaulOB's script.

It is a little different from the original slide-toggle script but it still works the same.


#39

Hey man, so to summarize the issue:

On this page (http://www.rayswoodworks.com/rwd-test/js-test/toggle-slide-2.html) in Chrome, when you manually resize the browser window to under 650px, you need to refresh the page before the toggle script runs correctly.

Did I get that right?


#40

Yes, that is correct!

It is the exact same thing that we went through in the other thread.
As seen in my screenshot of Chrome there.

I'm just trying to turn it into a Slide Toggle now.


#41

And that page there is using the menu-slide-toggle-2.js which was Andres_Vaquero's refinement of PaulOB's script that I mentioned in post #38.

The Slide Toggle demo page uses menu-slide-toggle.js which is a slightly different version developed earlier on this thread.

You can use which ever one is easiest to modify


#42

I don't want to clutter up the thread but if all else fails this codepen version of mine is now working. It's not pretty and simply adds a class to the html element so that you can have different toggle events for whether display:contents is supported or not.