-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Cartesian small domain #3404
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
Cartesian small domain #3404
Conversation
@@ -72,7 +72,7 @@ module.exports = function handlePositionDefaults(containerIn, containerOut, coer | |||
// in the axes popover to hide domain for the overlaying axis. | |||
// perhaps I should make a private version _domain that all axes get??? | |||
var domain = coerce('domain', dfltDomain); | |||
if(domain[0] > domain[1] - 0.01) containerOut.domain = dfltDomain; | |||
if(domain[0] > domain[1] - 0.001) containerOut.domain = dfltDomain; |
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.
Why 0.001
rather than even smaller, or dependent on height
or width
? Actually there’s a downside to depending on dimensions, that would mean some plots break if we export them smaller, particularly to make a thumbnail. So I’d maybe shrink the condition so small it’ll ALWAYS be less than a pixel for any reasonable graph size, and then test that nothing breaks if we really do make an axis that small (even if it can’t be seen).
@@ -72,7 +72,7 @@ module.exports = function handlePositionDefaults(containerIn, containerOut, coer | |||
// in the axes popover to hide domain for the overlaying axis. | |||
// perhaps I should make a private version _domain that all axes get??? | |||
var domain = coerce('domain', dfltDomain); | |||
if(domain[0] > domain[1] - 0.001) containerOut.domain = dfltDomain; | |||
if(domain[0] > domain[1] - (1 / 32768.0)) containerOut.domain = dfltDomain; |
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.
Where does that 32768
number come from?
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.
(No need for the trailing .0
in JS by the way)
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.
32768 = 2 ^ 15 is ofcourse the maximum signed short seems to be the maximum width/height on Chrome.
After more research I noticed this npm module updated today with good info! According to that we may reduce that to something like 16,384 for FF. Older versions of IE have lower limits.
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.
Interesting. Thanks for the info!
I don't adding a dependency for this is worth it. I suggest finding the max of min values listed.
Inspecting
looks like 1 / 4096
will be good enough.
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.
Done in a7c4485
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.
Thanks! Maybe you could put a comment above this line explaining where that 4096
comes from.
Ok. Nicely done 💃 |
Fix #3403