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(css-load): fix useCssLoadPatch determination when encountering unknown UA + platforms #417

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

booleanbetrayal
Copy link
Contributor

Requires rebuild post-merge.

Fixes #416

…known UA strings

Requires rebuild post-merge.

Fixes ocombe#416
@booleanbetrayal booleanbetrayal changed the title fix(css-load): fix useCssLoadPatch determination when encountering unknown UA strings fix(css-load): fix useCssLoadPatch determination when encountering unknown UA + platforms Feb 5, 2018
// Safari < 6
var versionMatch = ua.match(/version\/([\.\d]+)/i);
useCssLoadPatch = (versionMatch && versionMatch[1] && parseFloat(versionMatch[1]) < 6);
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in try-catch with no other changes

@booleanbetrayal
Copy link
Contributor Author

Wondering if you had chance to take a look at this @ocombe

@booleanbetrayal
Copy link
Contributor Author

any thoughts, @ocombe ? we're seeing this fail fairly frequently with the code that's in master.

@daerogami
Copy link

daerogami commented Mar 20, 2018

@booleanbetrayal Did you build and test this? I'm having a similar issue, but I have no idea what tool is used to build this.

Just manually added it to the main js file and minified version. Can confirm issue is fixed; however, my ocLazyLoad was also insanely outdated on v0.6.3

@booleanbetrayal
Copy link
Contributor Author

I haven't tried to build this but I could. Is that something I should do @ocombe ?

@booleanbetrayal
Copy link
Contributor Author

Added dist builds to this branch @daerogami via 0b785e6

@daerogami
Copy link

Awesome! It will be a week or so before I can get around to testing this on the project I'm using it on. Thanks @booleanbetrayal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants