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

Cartesian small domain #3404

Merged
merged 5 commits into from
Jan 10, 2019
Merged

Cartesian small domain #3404

merged 5 commits into from
Jan 10, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 6, 2019

Fix #3403

@archmoj archmoj added the bug something broken label Jan 6, 2019
@@ -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;
Copy link
Collaborator

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;
Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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

https://github.com/jhildenbiddle/canvas-size/blob/52382c95de0b66c447d032f3d8455c6aaaec1fc6/src/index.js#L55-L72

looks like 1 / 4096 will be good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a7c4485

Copy link
Contributor

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.

@etpinard
Copy link
Contributor

Ok. Nicely done 💃

@archmoj archmoj merged commit 05d0759 into master Jan 10, 2019
@archmoj archmoj deleted the fix3403-cartesian-small-domain branch January 10, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants