-
Notifications
You must be signed in to change notification settings - Fork 3k
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
touchevents: Use mq in touch event detection. #2309
Conversation
testStyles is unnecessarily slow if the browser supports matchMedia.
The Travis and AppVeyor failures look unrelated (and the Travis + node 6 run is green), but let me know if they're not. |
r? @ryanseddon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good change, just a small question on the other change
@@ -12,5 +12,5 @@ | |||
Detect support for Hover based media queries | |||
*/ | |||
define(['Modernizr', 'addTest', 'mq'], function(Modernizr, addTest, mq) { | |||
Modernizr.addTest('hovermq', mq(('(hover)'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw the extra parentheses while I was going through similar patterns. I can remove it if needed, but I thought a bit of cleanup never hurts. Let me know if you want to remove that commit from this PR and submit it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we're good, just making sure there wasn't an issue
@emilio @patrickkettner the PR #2294 would fix the Travis and AppVeyor failures |
* touchevents: Use mq in touch event detection. testStyles is unnecessarily slow if the browser supports matchMedia. * hovermq: remove stray parenthesis.
@emilio what's the status here? |
Its merged but not yet released @bholley |
Ok, great! I just remeasured, and confirmed that this fix still saves ~15ms on cnn pageload. @patrickkettner, any chance y'all could do another dot release (similar to #2291) so that we can get this into CNN's pipeline? |
would love to see a v3.7 out the door too, alas we are still behind with the release notes for 3.6 :-( |
testStyles + offsetTop is unnecessarily slow if the browser supports matchMedia.