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

Adding basic test for Cryptographic functions #1661

Merged
merged 1 commit into from Oct 20, 2015

Conversation

@roblarsen
Copy link
Contributor

@roblarsen roblarsen commented Sep 17, 2015

Closes #1659

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Sep 18, 2015

Looks like you've got some liniting errors

define(['Modernizr', 'prefixed', 'is'], function(Modernizr, prefixed, is) {
var crypto = prefixed('crypto', window);
var hasSubtle = false;
if ("subtle" in crypto) {

This comment has been minimized.

@ryanseddon

ryanseddon Sep 18, 2015
Member

New line above here

if ("subtle" in crypto) {
hasSubtle = true;
}

This comment has been minimized.

@ryanseddon

ryanseddon Sep 18, 2015
Member

Remove so only one new line



Modernizr.addTest('cryptography', hasSubtle);
});

This comment has been minimized.

@ryanseddon

ryanseddon Sep 18, 2015
Member

Make sure you have an eof

@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Sep 20, 2015

@roblarsen what browsers were you able to test in?

@roblarsen
Copy link
Contributor Author

@roblarsen roblarsen commented Sep 21, 2015

@patrickkettner What I have locally. I'll spin up some screen shots and compare with caniuse...

@roblarsen
Copy link
Contributor Author

@roblarsen roblarsen commented Sep 21, 2015

With the above fix:
https://www.browserstack.com/screenshots/2bd341cf7a590ec047bab12bc7a7757b2f170567

I think this is as expected across that whole pile of browsers

@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Sep 21, 2015

just a few nits

  1. following existing convention, this should be feature-detects/crypto.js
  2. I think that the detect name should be crypto
  3. squish down to a single commit plzandthnx
closes #1659
@roblarsen roblarsen force-pushed the roblarsen:crypto branch from c522fd1 to 802ca14 Sep 21, 2015
paulirish added a commit that referenced this pull request Oct 20, 2015
Adding basic test for Cryptographic functions
@paulirish paulirish merged commit 06bf811 into Modernizr:master Oct 20, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@roblarsen roblarsen deleted the roblarsen:crypto branch Mar 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.