Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix child menus for touch devices #666

Closed
wants to merge 1 commit into from
Closed

Conversation

sixhours
Copy link
Contributor

Add the ability to click on the parent once to access submenus, twice
to access the parent item. Props @iamtakashi for the original fix.

Add the ability to click on the parent once to access submenus, twice
to access the parent item. Props @iamtakashi for the original fix.
@obenland
Copy link
Member

Defining a function and then immediately calling it seems not very elegant. It would also be great for it to have a better documentation than simply stating that it's a fix.

Other than that is looks sound. :)
Where is it from?

touchStartFn = function( e ) {
var menuItem = this.parentNode;

if ( ! menuItem.classList.contains( 'focus' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classList isn't supported in IE9 unfortunately, the usage stats look fairly low for that browser, but it might be worth using the classname.indexof method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that. But I doubt there'll be a lot of mobile devices running IE9 or below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, to clarify the issue applies to mobile and desktop IE9, but that is going to be a pretty tiny number of users

@sixhours
Copy link
Contributor Author

Takashi would know for sure, but I believe it's based on the jQuery fix in Twenty Twelve.

@obenland
Copy link
Member

@iamtakashi ^^

@iamtakashi
Copy link
Contributor

Yes. The idea is based on the fix in Twenty Twelve: https://core.trac.wordpress.org/ticket/24767 but it's converted to native Javascript.

@samikeijonen
Copy link
Contributor

I only had iPad for testing. This works great in native Safari Browser but not in Chrome. I wonder if this is a bug in Chrome or in code.

@davidakennedy
Copy link
Contributor

Hey @sixhours, when you get a chance, I'd love to see a refresh on this.

I think addressing some of Obenland's feedback above would be great. Then we can test more thoroughly.

I'm in favor of this getting in though.

@sixhours
Copy link
Contributor Author

Defining a function and then immediately calling it seems not very elegant. It would also be great for it to have a better documentation than simply stating that it's a fix.

It's a bit over my head, honestly. If someone with more JS-fu wants to take a look at it, that'd be cool.

@sixhours
Copy link
Contributor Author

This works for me in testing! We've also used it in many themes on WordPress.com with success. I'll see if I can resolve the conflicts and merge it.

@mtomas7
Copy link
Contributor

mtomas7 commented May 26, 2016

@sixhours Did you use code from: #900 or the one that is attached to this issue?

@sixhours
Copy link
Contributor Author

Oh good catch, I meant to grab from #900. Will create a new PR; closing this one to avoid confusion.

@sixhours sixhours closed this May 26, 2016
@sixhours sixhours mentioned this pull request May 26, 2016
@sixhours sixhours deleted the fixMenuTouchTaps branch May 26, 2016 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants