Greedy-nav fix

[migupry]

This is a bug fix, and also an enhancement or feature.

Summary

I've identified 2 bugs related to the navbar and implemented fixes to those bugs, and I've changed the way the jquery.greedy-navigation script measures the available space for items, which results in consistency and precision:

1st bug:

Behavior:
On pages both with and without logos, though rarely, the "visible-link" "ul" object takes more space than it should, making some items on the navbar appear completely or partially hidden.
The cause:
On the _base.scss file, it is set both "-webkit-transition" and "transition" attributes for a bunch of elements, and importantly to our case, the "a" elements and the "img" elements.
On the Navbar there are 3 types of "a", which are affected by transitions and are relevant to this bug: "site-logo", "site-title" and ".visible-links -> li -> a" (which are the navbar visible section links) also, the "img" object under "site-logo".
On the greedy navigation script, jquery.greedy-navigation.js, the line availableSpace = $vlinks.width() - 10; measures vlinks (the ".visible-links" "ul" object) width, and this calculation is crucial to determine how many items may fit on the navbar. After some debugging, I've found out that the measured size of vlinks was not consistent, which was... strange. I thought this had to do with the script, but nothing seemed to be wrong there.
Then finally it clicked to me! It was a race condition. Most of the times, the measurement is done after the transition on the "a" (and img) objects width is done, but yeah, most of the times is not always. Sometimes, the vlinks object's width is measured before or during the transition, and thus getting the wrong value.
The fix:
We could wait for the transition to occur in order to execute the greedy navigation script logic (relevant related discussion here), but... I mean... Is it worth it? We would have to add a considerable amount of complexity to the code in order to control the transition's execution with js/jquery.
My suggestion is simply: "transitions on navbar -> bad":
diff between my fix and original code:
_navigation.scss

187,188d186 // .greedy-nav a
<     -webkit-transition: none;
<     transition: none;
202,206d199 // .greedy-nav img
<   }
<   
<   img{
<     -webkit-transition: none;
<     transition: none;

note: .visible-links a::before elements also have a transition, but they don't seem to influence the code, so I've ignored it.
My conscience says it would be better to disable all transitions on the navbar, since it would make harder for an user customization on the template or even, who knows, an update, to bring this bug back again. But this should work for now.

2nd bug:

Behavior:
On pages with logos, not so rarely, the "visible-link" "ul" object takes more space than it should, making some items on the navbar be completely or partially hidden. The same effect as the first bug, but more frequently, and limited to pages with logos.
The cause:
Although we've target both the .site-logo element and the img element inside of it on the first solution, this is not enough to fix this. After some testing, I've noticed yet another race condition: similar as before, the vlinks measurement can occur before the .site-logo img is even loaded. If the image happens to take a little more time to load, since the widths are not fixed, the vlinksobject is measured without it, and when the image is finally loaded, all the calculation with the wrong values is already done.
The fix:
We need to wait for the images on the navbar to load before doing the relevant calculation. As discussed here, the best way to make this happen without much hassle seems to be using David DeSandro’s imagesLoaded library since "it has no dependencies, is pretty small, and works with jQuery".

I've come up with something studying imagesloaded and relevant discussions here and here.

This doesn't add any dependencies and simply doesn't do anything if the page doesn't have a logo.

If it does have a logo, it checks if it's loaded before continuing:

• It the logo is loaded, it proceeds with the script as usual.
• If it's not loaded, it uses jQuery to wait for the image to load/fail before continuing with the script.

I've tested with Chrome/Firefox mobile browsers, as well as Instagram and Telegram's in-app browsers and it worked 100% of the time, even if the logo has a broken src.

It should work with legacy browsers as well.

diff assets/js/plugins/jquery.greedy-navigation.js ~/minimal-mistakes/assets/js/plugins/jquery.greedy-navigation.js 
72c72
<   var logoImg = $('nav.greedy-nav .site-logo img');
---
>   check();
74,84d73
<   // check if page has a logo
<   if(logoImg.length !== 0){
<     // check if logo is not loaded
<     if(!(logoImg[0].complete || logoImg[0].naturalWidth !== 0)){
<       // if logo is not loaded wait for logo to load or fail to check
<       logoImg.one("load error", check);
<     // if logo is already loaded just check
<     } else check();
<   // if page does not have a logo just check
<   } else check();
<   

Then I faced something that had me boggled since I've started analyzing the greedy-nav script: The way it calculates the available space for links on the navbar. The relevant line is availableSpace = $vlinks.width() - 10;. So what this does is to presume that the available space for items in the navbar is the totality of the width of visible links element minus 10px. I had in my mind this was strange because 10 pixels is a static value, that couldn't take into account the dynamic nature of css with it's media queries and so on.

The width of the visible links element decreased by 10 pixels is an educated guess of the available space, but it is not precise. The developers of greedy-nav are certainly not to blame, as doing precise calculations adds a considerable amount of complexity, and in my opinion, defeats the purpose of it as a versatile and lightweight priority+ navbar implementation.

If you want to calculate the precise available space for navItems, you need to take the navbar inner width and decrease from it the width of every visible element (except for the links). But how different the precise measurement and the approximation could be, you may ask? On a page without a logo, I've measured a 10px difference on a 360px width Android device, and a remarkable 30px difference on a 320px width Iphone device. Thats not at all insignificant as it represents +9% of the Iphone device width and can easily result in consistent glitches on some devices in some pages (as in my test case).

As we're talking about precision (and good use of space), there's 2 another issues.

• While there's no hidden item behind the "hamburger menu toggle", you don't need to take the toggle's width into account, but as soon as there is at least 1 hidden item, you need to account for it. Handling this maximizes the use of navbar space and adds precision.
• Minimal mistakes has 4 CSS width breakpoints (<768px, <1024px, < 1280px, >= 1280px), and under each of this breakpoints the size of navbar items change. Since in the original script the calculation of breakpoints for the navbar (a list of points when the items should go into the 'hamburger menu') only happens once the script loads, the navbar start getting inconsistent once you resize your window and reach another CSS width breakpoint (thus resulting in the navbar items changing width). This could occur when you rotate a large mobile or tablet screen, or when you maximize/restore a window on Desktop. Recalculating the navbar breakpoints once (and only once) you reach another CSS width breakpoint is also necessary if we're aiming for precision

So I've implemented in the script all those things I've mentioned. I've done my best, doing performance tests all along, to make this the less performance greedy as I could. This of course inevitably added time complexity to the code, since I've added some grabby calculations (particularly elements widths measurements). That being said, at least in my test cases the added complexity is not at all performance critical, nor can it be perceived by the user, or outside of performance testing. This is tested both with and without the logo and the search toggle. I'm proud to say this adds what I call "pixel perfect responsivity". :)

diff jquery.greedy-navigation.js ~/minimal-mistakes/assets/js/plugins/jquery.greedy-navigation.js 
12,34d11
<   var $nav = $("nav.greedy-nav");
<   var $logo = $('nav.greedy-nav .site-logo');
<   var $logoImg = $('nav.greedy-nav .site-logo img');
<   var $title = $("nav.greedy-nav .site-title");
<   var $search = $('nav.greedy-nav button.search__toggle');
<   
<   var numOfItems, totalSpace, closingTime, breakWidths;
< 
<   // This function measures both hidden and visible links and sets the navbar breakpoints
<   // This is called the first time the script runs and everytime the "check()" function detects a change of window width that reached a different CSS width breakpoint, which affects the size of navbar Items 
<   // Please note that "CSS width breakpoints" (which are only 4) !== "navbar breakpoints" (which are as many as the number of items on the navbar)
<   function measureLinks(){
<     numOfItems = 0;
<     totalSpace = 0;
<     closingTime = 1000;
<     breakWidths = [];
< 
<     // Adds the width of a navItem in order to create breakpoints for the navbar
<     function addWidth(i, w) {
<       totalSpace += w;
<       numOfItems += 1;
<       breakWidths.push(totalSpace);
<     }
36,49c13,16
<     // Measures the width of hidden links by making a temporary clone of them and positioning under visible links
<     function hiddenWidth(obj){
<       var clone = obj.clone();
<       clone.css("visibility","hidden");
<       $vlinks.append(clone);
<       addWidth(0, clone.outerWidth());
<       clone.remove();
<     }
<     // Measure both visible and hidden links widths
<     $vlinks.children().outerWidth(addWidth);
<     $hlinks.children().each(function(){hiddenWidth($(this))});
<   }
<   // Get initial state
<   measureLinks();
---
>   var numOfItems = 0;
>   var totalSpace = 0;
>   var closingTime = 1000;
>   var breakWidths = [];
51,53c18,23
<   var winWidth = $( window ).width();
<   // Set the last measured CSS width breakpoint: 0: <768px, 1: <1024px, 2: < 1280px, 3: >= 1280px.
<   var lastBreakpoint = winWidth < 768 ? 0 : winWidth < 1024 ? 1 : winWidth < 1280 ? 2 : 3;
---
>   // Get initial state
>   $vlinks.children().outerWidth(function(i, w) {
>     totalSpace += w;
>     numOfItems += 1;
>     breakWidths.push(totalSpace);
>   });
59,66d28
<     winWidth = $( window ).width();
<     // Set the current CSS width breakpoint: 0: <768px, 1: <1024px, 2: < 1280px, 3: >= 1280px.
<     var curBreakpoint = winWidth < 768 ? 0 : winWidth < 1024 ? 1 : winWidth < 1280 ? 2 : 3;
<     // If current breakpoint is different from last measured breakpoint, measureLinks again
<     if(curBreakpoint !== lastBreakpoint) measureLinks();
<     // Set the last measured CSS width breakpoint with the current breakpoint
<     lastBreakpoint = curBreakpoint;
< 
67a30
>     availableSpace = $vlinks.width() - 10;
69,74d31
<     // Decrease the width of visible elements from the nav innerWidth to find out the available space for navItems
<     availableSpace = /* nav */ $nav.innerWidth()
<                    - /* logo */ ($logo.length !== 0 ? $logo.outerWidth(true) : 0)
<                    - /* title */ $title.outerWidth(true)
<                    - /* search */ ($search.length !== 0 ? $search.outerWidth(true) : 0)
<                    - /* toggle */ (numOfVisibleItems !== breakWidths.length ? $btn.outerWidth(true) : 0);
82,83c39,40
<       // There is more than enough space. If only one element is hidden, add the toggle width to the available space
<     } else if (availableSpace + (numOfVisibleItems === breakWidths.length - 1?$btn.outerWidth(true):0) > breakWidths[numOfVisibleItems]) {
---
>       // There is more than enough space
>     } else if (availableSpace > breakWidths[numOfVisibleItems]) {

Context

Fixes #2664

[stale]

This issue has been automatically marked as stale because it has not had recent activity.

If this is a bug and you can still reproduce this error on the master branch, please reply with any additional information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

[mmistakes]

Not stale.