I have the following front-end code for a responsive navigation menu which I thought using in my WordPress website.
Clicking the .burger
element should cause adjacent .menu
element to appear or disappear, in dropdown or dropup respectively, but without a breakpoint conflict such as this, which I suffer from:
- We open a browser window
<=959px
and open the menu. - We resize the window to
>=960px
and then we resize back to<=959px
. - We see that the menu is still open and isn't restarted.
Why I seek review
Many programmers advised me to take a pure CSS approach (no JavaScript) but in this case I create my markup with a WordPress page builder (Elementor) hence it feels inefficient to me. I also feel my JavaScript is too long and a bit confusing.
You might know a simpler and shorter (more methodal and more intuitive) pattern in vanilla JavaScript available in 2018 (ES6/ES7?) or in jQuery 3.x.x.
Code
document.addEventListener('DOMContentLoaded', ()=>{
let clicks = 0;
let menu = document.querySelector('#menu-primary');
let burger = document.querySelector('.burger');
let isMenuVisible = false;
burger.addEventListener('click', ()=>{
isMenuVisible = !isMenuVisible;
menu.style.display = isMenuVisible ? 'block' : 'none';
});
let mobileBehavior = ()=>{
menu.style.display = 'none';
};
if (window.innerWidth <= 959) {
mobileBehavior();
}
window.addEventListener('resize', ()=>{
if (window.innerWidth <= 959) {
clicks = 1;
} else if (window.innerWidth >= 960) {
menu.style.display = 'block';
}
});
});
.burger {display: block; text-align: center; color: var(--w); margin-bottom: 0 !important; font-weight: bold}
#menu-primary {display: none}
@media screen and (min-width: 960px) {
.burger {display: none}
#menu-primary {display: block}
}
<div class="burger">BARS</div>
<ul id="menu-primary">
<li>Homepage</li>
<li>Contact_us</li>
</ul>
Update
I reported a problem with the code in this StackOverflow session; the problem is that the code can conflict with some page builders and does conflict with Elementor WordPress-Drupal page builder, as described there.
-
1\$\begingroup\$ "advised me to take a pure CSS approach but I create my markup with a WordPress page builder and it feels to me inefficient" Why is it inefficient to use a combination of CSS and JavaScript? Is the markup and the CSS fixed or can anything be changed? Should only the JavaScript be reviewed? \$\endgroup\$insertusernamehere– insertusernamehere2018年08月28日 15:55:14 +00:00Commented Aug 28, 2018 at 15:55
-
\$\begingroup\$ The combination is not inefficient --- using only css in this case would be inefficient as I don't have comfortable control over the markup. In my case above only the JavaScript should be reviewed. \$\endgroup\$user125391– user1253912018年08月28日 18:18:13 +00:00Commented Aug 28, 2018 at 18:18
-
1\$\begingroup\$ Normally after receiving answers you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information. I have updated my answer accordingly but please don't edit the code anymore. \$\endgroup\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2018年08月28日 22:11:49 +00:00Commented Aug 28, 2018 at 22:11
-
\$\begingroup\$ Does the code work as intended? As in, did you fix the problem presented earlier to your satisfaction? \$\endgroup\$Mast– Mast ♦2018年08月29日 09:10:58 +00:00Commented Aug 29, 2018 at 9:10
-
\$\begingroup\$ @Mast I have yet to test the code seriously (Sam's answer seems promising and I need to understand the code better first). I haven't fixed that problem. \$\endgroup\$user125391– user1253912018年08月29日 15:50:44 +00:00Commented Aug 29, 2018 at 15:50
1 Answer 1
General feedback
Overall it seems somewhat simple and works fine except for the issue I saw you mentioned in a comment on the accepted answer to the related Stack Overflow post, about having to click the menu button twice after resizing it that is caused by a conflict with page builder as explained in question update above.
One thing to consider is getting rid of the variable isMenuVisible
- just check the value of menu.style.display
when updating the visibility of the menu.
Suggestions
const
vs let
I would use const
for any value that should not get re-assigned - e.g. menu
, burger
. That way if you inadvertently re-assign a value to those, an error will be thrown.
Identifying the element used to open the menu
If there really is only one element for opening the menu, why not use an id attribute to find that instead of a class name.
Querying DOM for elements
Using document.getElementById()
to access those DOM elements will be quicker1 than using document.querySelector()
.
Variable clicks
The variable clicks
doesn't appear to be used - just set to 0
initially and then updated during the resize event callback. Is that leftover from other code? Or does it control something else in your code?
simpler and shorter patterns
As far as shortcuts using ecmascript-6 features, the only things I could think of would be to remove the block on mobileBehavior
(to make it a single line) and using a single argument with only one character (e.g. named something like _
) for the arrow functions instead of an empty set of parentheses (though that only saves character for each function).
Method mobileBehavior()
and the resize event callback
Since mobileBehavior()
is only called once, that functionality could just be moved to the place it is called... Then it doesn't really make sense to have that function. Perhaps it would be suitable to define a function like setDisplayBasedOnWidth
, which contains most of the functionality of the resize callback (except setting the display to none
when the width is less than the threshold). Then that function can be called initially, as well as after a resize event.
See the updated code below for slight improvements adapted.
document.addEventListener('DOMContentLoaded', _ => {
const menu = document.getElementById('menu-primary');
const burger = document.getElementById('burger');
burger.addEventListener('click', _ => {
const currentDisplay = menu.style.display;
menu.style.display = currentDisplay == 'none' ? 'block' : 'none';
});
const setDisplayBasedOnWidth = _ => menu.style.display = window.innerWidth <= 959 ? 'none' : 'block';
window.addEventListener('resize', setDisplayBasedOnWidth);
setDisplayBasedOnWidth();
});
:root {
--w: #00ff00;
}
#burger {
display: block;
text-align: center;
color: var(--w);
margin-bottom: 0 !important;
font-weight: bold
}
#menu-primary {
display: none
}
@media screen and (min-width: 796px) {
.burger {
display: none
}
#menu-primary {
display: block
}
}
<div id="burger">BARS</div>
<ul id="menu-primary">
<li>Homepage</li>
<li>Contact_us</li>
</ul>
-
\$\begingroup\$ Sadly it seems not to work on my site (integrative-massage.co.il); The menu won't open in mobile mode. \$\endgroup\$user125391– user1253912018年08月30日 09:54:05 +00:00Commented Aug 30, 2018 at 9:54
-
\$\begingroup\$ Although here it works fine. \$\endgroup\$user125391– user1253912018年08月30日 10:07:05 +00:00Commented Aug 30, 2018 at 10:07
-
\$\begingroup\$ Hmm... I doubt this is the cause but I do see an error in the
<script>
tag before the menu script tag:document.querySelector('.entry-meta').innerText is null
- apparently there are no elements with that class name on the page... \$\endgroup\$2018年08月30日 16:42:45 +00:00Commented Aug 30, 2018 at 16:42 -
\$\begingroup\$ Will check later. Hard period. \$\endgroup\$user125391– user1253912018年08月30日 17:42:35 +00:00Commented Aug 30, 2018 at 17:42
-
\$\begingroup\$ Seems the code I had was working fine and the problem came from a page builder for WordPress that altered the native behavior I added. Thanks ! \$\endgroup\$user125391– user1253912018年11月06日 19:36:24 +00:00Commented Nov 6, 2018 at 19:36