Skip to content
This repository has been archived by the owner on Dec 1, 2019. It is now read-only.

Fix ability to expand nav submenus on large-viewport touch interfaces #971

Merged
merged 6 commits into from
Nov 10, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Nov 7, 2019

In #949 changes were merged to fix #790, the inability to access a submenu item of the Desktop Horizontal Menu on tablet (>1000px). However, I found the fix is not working:

image

An alternative approach to adding JS to support touch interfaces is rather to use CSS, specifically media queries for interaction media features (caniuse, currently 92.57% of browsers globally).

An added benefit of using CSS here is that it will ensure that the nav menus work when JS is turned off in the browser (or it fails to load, or it is an AMP page). Nevertheless, since IE11 doesn't support this media query, perhaps some JS logic should be included to add a touch-enabled class name on the body and then add CSS rules for them as well (for IE11 users who have JS enabled: <7.43% of global users).

Props to @pierlon who found this solution when devising an AMP-compatible way to make the submenus accessible on large-viewport touch interfaces: ampproject/amp-wp#3696.

style.css Show resolved Hide resolved
style.css Outdated Show resolved Hide resolved
@pattonwebz
Copy link
Member

pattonwebz commented Nov 7, 2019

With this change here we rely on CSS and the interaction media queries feature but there is no support for that in IE 11 or before. About 1.85% global users are on IE11 according to the CanIUse stats.

I am interested to hear thoughs about how best to handle that. I don't think the figure is small enough to ignore it but at the same time I do not want to have to introduce any kind of a JS shim for IE because this PR actually removes some JS code making the menus a little bit easier to work with from a developers perspective.

@westonruter westonruter force-pushed the fix/large-viewport-touch-nav-submenu-expansion branch from bd37f06 to d5f02f0 Compare November 7, 2019 21:17
@westonruter
Copy link
Member Author

@pattonwebz I've added a polyfill for IE11 in 6f20c02.

@pattonwebz
Copy link
Member

I see that, I am just trying to boot my laptop into windows so I can give this a live test on IE 11 but otherwise this seems good to me.

After some googling I was not able to find any solution other than a polyfill. It is a shame but that is just what we have to do to keep IE working 🤷‍♂️

@carolinan
Copy link
Contributor

I am not able to reproduce this JavaScript error.

In #949 changes were merged to fix #790, the inability to access a submenu item of the Desktop Horizontal Menu on tablet (>1000px). However, I found the fix is not working:

image

An alternative approach to adding JS to support touch interfaces is rather to use CSS, specifically media queries for interaction media features (caniuse, currently 92.57% of browsers globally).

An added benefit of using CSS here is that it will ensure that the nav menus work when JS is turned off in the browser (or it fails to load, or it is an AMP page). Nevertheless, since IE11 doesn't support this media query, perhaps some JS logic should be included to add a touch-enabled class name on the body and then add CSS rules for them as well (for IE11 users who have JS enabled: <7.43% of global users).

Props to @pierlon who found this solution when devising an AMP-compatible way to make the submenus accessible on large-viewport touch interfaces: ampproject/amp-wp#3696.

@pattonwebz pattonwebz merged commit db82df5 into master Nov 10, 2019
@pattonwebz pattonwebz deleted the fix/large-viewport-touch-nav-submenu-expansion branch November 10, 2019 22:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to access a submenu item of the Desktop Horizontal Menu on tablet (>1000px)
3 participants