Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 10, 2019
Merged

Cartesian small domain #3404

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7fccac
fix 3403 small domains added a new mock
archmoj Jan 6, 2019
40f2a1c
new baseline
archmoj Jan 6, 2019
e8f8fd0
apply a smaller const
archmoj Jan 7, 2019
a7c4485
using 4096
archmoj Jan 9, 2019
5da2778
added a comment to explain why 4096 may be a good const to apply
archmoj Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/plots/cartesian/position_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Lib.noneOrAll(containerIn.domain, containerOut.domain, dfltDomain);
}

Expand Down