Pulling in feature-detection tests from modernizr-plugins #419

Merged
merged 8 commits into from Dec 4, 2011

Projects

None yet

5 participants

@addyosmani

This repository contains additional feature-tests for Modernizr covering the following:

  • DataView API
  • Microdata
  • Mozilla Audio API
  • WebAudio API
  • Track element
  • getUserMedia/device API
  • GamePad API
  • EventSource API
  • Animated PNG
  • Progress element
  • ClassList

It also has a test for ContextMenu support (which I've excluded as @codepo8 pushed something similar recently) and a combined test for both Mozilla and WebKit's Audio APIs (which I'm excluding unless it's specifically asked for).

At this time it would be awesome if someone could do a code-review to make sure these tests are solid.

@paulirish
Modernizr member

i think the filenames need some tweaking otherwise this is looking strong :)

thx for the audio test changes :)

@addyosmani

My pleasure! :)

I'll try tweaking the filenames up a little sooon.

@ryanseddon
Modernizr member

I saw these the other day and was going to ask you to do a pull request, awesome work.

The test names should probably all be lowercase to be inline with the others plugins (we should document that in docs on what is preferred?).

I'm getting errors with the apng test not liking the following line because of the preceding cast to bool operator being outside the parentheses.

if !! (typeof canvas.getContext == 'undefined') {

Also since this is an async test I'd put the the Modernizr.addTest call inside the onload event. The same way the webp plugin does it.

@addyosmani

@ryanseddon How does the updated PR look?

There are a few different ways the async test could be done, but I figured that wrapping the load/onerror event handler attachment inside the context test would avoid a little overhead. If you have a preferred alternative structure to it, let me know and I'll happily update further.

Cheers!

@addyosmani

As requested by @paulirish, I've updated the PR to include the additional tests I added to the other repo today.

(thanks to @mathiasbynens and @timmywil for the head's up on naming)

@paulirish
Modernizr member
@ryanseddon
Modernizr member

Looks good.

What I was saying about the async test was to do the addTest inside the onload as that covers both passing and failing. I'm not sure if the onerror will ever fire in any browser?

@mathiasbynens

As @ryanseddon said:

The test names should probably all be lowercase to be inline with the others plugins (we should document that in docs on what is preferred?).

All tests on http://modernizr.github.com/Modernizr/test/ have lowercase names.

@davidmurdoch

addTest toLowerCase()s feature names: addTest("featureName", bool) => Modernizr.featurename === bool

@mathiasbynens

@davidmurdoch Exactly; it would be much less confusing to lowercase the test names manually for people reading the feature test source code.

@davidmurdoch

Agreed.

aside: addTest should probably not enforce casing for feature names at all. The "official" Modernizr policy could be lowercase - but devs should be free to use whatever convention they like. Right? (Line 863 as well as addTest would need to be updated).

@addyosmani
@addyosmani

Cases updated <3

can someone review?

@paulirish
Modernizr member

im gonna tweak these filenames a touch more and kill some superfluous !! casting since we're using in operator.
followups coming..

but THANK YOU Addy! great stuff. I think this kills a few outstanding things in the issue tracker too.

@paulirish paulirish merged commit 75afc46 into Modernizr:master Dec 4, 2011
@addyosmani

My pleasure @paulirish! Glad they could come in useful for something :)

@patrickkettner patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015
@paulirish paulirish renaming some of the files from addy's commit #419 f37030c
@patrickkettner patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015
@paulirish paulirish simplification of some of addy's new tests in #419 4149622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment