-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 3-D config for webgl buffer on iPad Pro & iPad 7th + iOs v13 + Safari #4546
Conversation
@@ -82,7 +82,7 @@ | |||
"gl-mat4": "^1.2.0", | |||
"gl-mesh3d": "^2.3.0", | |||
"gl-plot2d": "^1.4.3", | |||
"gl-plot3d": "^2.4.1", | |||
"gl-plot3d": "^2.4.2", |
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.
result === false && | ||
userAgent.indexOf('Macintosh') !== -1 && | ||
userAgent.indexOf('Safari') !== -1 && | ||
navigator.maxTouchPoints > 1 |
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.
How did you find this solution?
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.
src/plots/gl3d/scene.js
Outdated
var tablet = isTablet(); | ||
|
||
function isTablet() { | ||
var navigator = window.navigator; |
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.
Does is-mobile
setup a guard for cases when window.navigator
is undefined?
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.
Good call.
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 don't see any check there concerning navigator
.
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.
OK, can you add one just to be sure?
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 wouldn't someone running plotly.js in some sort of weird environment have their app break in a patch release)
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.
Actually it is added in 436fca4 here:
if(!ua && typeof navigator !== 'undefined') ua = navigator.userAgent;
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.
Sure, we can still get breakage on this line below:
navigator.maxTouchPoints > 1
if window.navigator
isn't defined.
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.
Addressed in aa28eba.
src/plots/gl3d/scene.js
Outdated
|
||
function isTablet() { | ||
var navigator = window.navigator; | ||
var userAgent = navigator.userAgent; |
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.
and similarly here, if is-mobile
guards against undefined navigator.userAgent
, we should too here.
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.
Good call.
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.
Revised in 436fca4.
Sweet. Thanks very much 💃 - once tests pass! |
@etpinard Thanks for the review. |
PR to |
Fixes #4502 | Demo
@plotly/plotly_js
cc: @jackparmer