Ie8 compat

[anselmdk]

I'm was just testing my site in IE8, and ran into the issue that betternavigator breaks the JS in IE8, I'd prefer if we could just remove betternavigator entirely when dealing with IE8. See my suggestion here.

[jonom]

Hi @anselmdk thanks for the feedback and PR, sorry for the slow reply. I would prefer if the menu was available in IE8 but we just lose the collapse/expand functionality. If that's okay with you would you mind altering your PR to remove the else section (keep the if) and change the default class of the #BetterNavigator div to open instead of collapsed in BetterNavigator.ss. I think that should do it. Then if you could squash the commits that would be great.

Oh, would also need to change

    //Restore menu state
    if (getCookie('BetterNavigator') === 'open') {
        BetterNavigator.className = 'open';
    }

to

    //Restore menu state
    if (getCookie('BetterNavigator') === 'collapsed') {
        BetterNavigator.className = 'collapsed';
    }

If you're not available to make these changes let me know and I can implement them.

[jonom]

p.s. then in your own sites you could do

<!--[if gt IE 8]><!-->
    $BetterNavigator
<!--<![endif]-->

to hide the menu from IE8

[anselmdk]

@jonom good ideas. I just did those changes (I'll be happy to squash the pr in the end). For now, one comment. I think this will imply for the navigator to be open by default, maybe we should add in a line that for all devices that support document.addEventListener, if no cookie is set the navigator should toggle itself?

[jonom]

Yeah I think that makes sense. So I guess we could just do

//Restore menu state if set, default to collapsed
if (getCookie('BetterNavigator') != 'open') {
    BetterNavigator.className = 'collapsed';
}

[anselmdk]

I tried that, but that made the nav flicker on each page load as it would start by being open, and would collapse only when the js kicked in. I feel it's better to keep the nav collapsed by default and to specifically open it for IE8 or lower. Here's my suggestion: https://github.com/anselmdk/silverstripe-betternavigator/commit/cfbeb60c1cb5e0764619874724a3838ae833feb8

[jonom]

Yeah I think the flicker is the reason I defaulted it to collapsed in the beginning actually.

Is document.addEventListener the only thing that IE8 doesn't support in the script? In that case I don't think we need to limit the functionality in IE8, we could do something like this:

initialiseBetterNavigator() {
  //add onClick and cookie functionality
}
if (document.addEventListener) {
  document.addEventListener("DOMContentLoaded", function () {
    //wait til DOM is ready
    initialiseBetterNavigator();
  }
}
else {
  //initialise straight away - fine if script is loaded after BN dom element
  initialiseBetterNavigator();
}

I'm thinking that the change you proposed assumes we have access to the DOM, and if we're making that assumption we may as well add the toggle functionality instead of just making it visible. I haven't tested this though...

[anselmdk]

That worked. Not it's working in IE8! If you've got no more comments, I'll squash those commits.

[jonom]

Looks great, thanks @anselmdk. Yes if you can squash the commits please I'll merge it in after that.

[anselmdk]

@jonom I think that should be it!

[jonom]

Thanks @anselmdk :)

[anselmdk]

It's been my pleasure!