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

cleanup browser tests and fix null reference check on window.documentElement.style.WebkitAppearance #447

Merged
merged 1 commit into from Apr 27, 2017

Conversation

@thebigredgeek
Copy link
Collaborator

@thebigredgeek thebigredgeek commented Apr 26, 2017

No description provided.

…Element.style.WebkitAppearance
@thebigredgeek
Copy link
Collaborator Author

@thebigredgeek thebigredgeek commented Apr 26, 2017

@TooTallNate can you do a sanity check on this for me?

@thebigredgeek
Copy link
Collaborator Author

@thebigredgeek thebigredgeek commented Apr 26, 2017

also fixes #436

@coveralls
Copy link

@coveralls coveralls commented Apr 26, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling 7ab38e4 on chore/cleanupBrowserChecks into f311b10 on master.

@thebigredgeek thebigredgeek merged commit cae07b7 into master Apr 27, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 63.75%
Details
@thebigredgeek thebigredgeek deleted the chore/cleanupBrowserChecks branch Apr 27, 2017
@darrachequesne
Copy link

@darrachequesne darrachequesne commented Apr 27, 2017

@thebigredgeek I think this has broken webworker compatibility (introduced by #382)

@thebigredgeek
Copy link
Collaborator Author

@thebigredgeek thebigredgeek commented Apr 27, 2017

@darrachequesne whoops! That was sloppy of me! I knew I should have waited for the sanity check from @TooTallNate haha. I just deprecated 2.6.5 and released 2.6.6 with a fix. You should be able to unpin

@darrachequesne
Copy link

@darrachequesne darrachequesne commented Apr 28, 2017

@thebigredgeek great, thanks!

@@ -40,20 +40,20 @@ function useColors() {
// NB: In an Electron preload script, document will be defined but not fully
// initialized. Since we know we're in Chrome, we'll just detect this case
// explicitly
if (typeof window !== 'undefined' && window && typeof window.process !== 'undefined' && window.process.type === 'renderer') {
if (window && window.process && window.process.type === 'renderer') {

This comment has been minimized.

@TooTallNate

TooTallNate Apr 28, 2017
Member

The typeof window !== 'undefined' stuff is important, because that will return false if window is not defined, but simply window will throw an error if it is not defined. All of the root-level variables should stick with the typeof check IMO. Once that first check passes then checking for thuthyness should be enough for the properties after that.

return true;
}

// is webkit? http://stackoverflow.com/a/16459606/376773
// document is undefined in react-native: https://github.com/facebook/react-native/pull/1632
return (typeof document !== 'undefined' && document && 'WebkitAppearance' in document.documentElement.style) ||
return (document && document.documentElement && document.documentElement.style && document.documentElement.style.WebkitAppearance) ||

This comment has been minimized.

@TooTallNate

TooTallNate Apr 28, 2017
Member

This is the only really important line it seems like. For the purposes of fixing #436, maybe we can narrow the bug fix commit to just a one-liner commit. And perhaps address other styling fixes for this code separately.

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented Apr 28, 2017

Damnit, this GitHub Reviews feature tripped me up. First time using it. I had these comments written out 2 days ago and I thought they were visible. Turns out I had to hit the "Submit Review" button first. My bad.

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.