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.

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

Jump to bottom

Fix for issues #3917, #4380, #2966 - log scale with min: 0 #4913

Merged
merged 5 commits into from Nov 10, 2017
Merged

Fix for issues #3917, #4380, #2966 - log scale with min: 0 #4913

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b7421d
Fix for issues #3917, #4380, #2966
jcopperfield Nov 1, 2017
4de3227
Add: Requested code style changes for getPixelForValue and getValueFo…
jcopperfield Nov 6, 2017
165ad97
Fix: could not minify due to invalid function declaration
jcopperfield Nov 6, 2017
3519612
Fix: renamed variables to improve readability
jcopperfield Nov 6, 2017
c5e24a6
Fix: prefix private function with _ and correct jsdocs
jcopperfield Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
116 changes: 74 additions & 42 deletions src/scales/scale.logarithmic.js
View file
Open in desktop
Expand Up @@ -179,62 +179,94 @@ module.exports = function(Chart) {
},
getPixelForValue: function(value) {
var me = this;
var start = me.start;
var newVal = +me.getRightValue(value);
var opts = me.options;
var tickOpts = opts.ticks;
var innerDimension, pixel, range;
var realValue = +me.getRightValue(value);
var reverse = me.options.ticks.reverse;
var lg = helpers.log10;
Copy link
Member

Choose a reason for hiding this comment

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

I would rename it log10 to make it more obvious when reading line 227

var exp = Math.floor(lg(me.minNotZero));
var significand = Math.floor(me.minNotZero / Math.pow(10, exp));
var firstTickValue = significand * Math.pow(10, exp);
var offset = 0;
var baseValue, innerDimension, pixel, range, sign;

if (reverse) {
range = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid nesting start and end under range? (faster, lighter and easier to read)

start = me.end;
end = me.start;
sign = -1;

start: me.end,
end: me.start
};
sign = -1;
} else {
range = {
start: me.start,
end: me.end
};
sign = 1;
}
baseValue = range.start;
if (me.isHorizontal()) {
range = helpers.log10(me.end) - helpers.log10(start); // todo: if start === 0
if (newVal === 0) {
pixel = me.left;
} else {
innerDimension = me.width;
pixel = me.left + (innerDimension / range * (helpers.log10(newVal) - helpers.log10(start)));
}
innerDimension = me.width;
pixel = (reverse)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't need to wrap reverse in parentheses. I also would probably put this on just one line given that it wouldn't be too long of a line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO It improves readability.
I don't however want to start a discussion over code style, so if it is a must I will adjust the code as requested.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @benmccann, I would use parenthesis only when it's needed but also I'm not sure useless parenthesis are removed by the minifier.

About the "one line" style, it's not a "must" (else it would be enforced) but most of the code uses the a ? b : c form when a, b and c are short enough to make easier to read when on one line (which, I agree, is not necessary perceived the same by everyone).

? me.right
: me.left;
} else {
// Bottom - top since pixels increase downward on a screen
innerDimension = me.height;
if (start === 0 && !tickOpts.reverse) {
range = helpers.log10(me.end) - helpers.log10(me.minNotZero);
if (newVal === start) {
pixel = me.bottom;
} else if (newVal === me.minNotZero) {
pixel = me.bottom - innerDimension * 0.02;
} else {
pixel = me.bottom - innerDimension * 0.02 - (innerDimension * 0.98 / range * (helpers.log10(newVal) - helpers.log10(me.minNotZero)));
}
} else if (me.end === 0 && tickOpts.reverse) {
range = helpers.log10(me.start) - helpers.log10(me.minNotZero);
if (newVal === me.end) {
pixel = me.top;
} else if (newVal === me.minNotZero) {
pixel = me.top + innerDimension * 0.02;
} else {
pixel = me.top + innerDimension * 0.02 + (innerDimension * 0.98 / range * (helpers.log10(newVal) - helpers.log10(me.minNotZero)));
}
} else if (newVal === 0) {
pixel = tickOpts.reverse ? me.top : me.bottom;
} else {
range = helpers.log10(me.end) - helpers.log10(start);
innerDimension = me.height;
pixel = me.bottom - (innerDimension / range * (helpers.log10(newVal) - helpers.log10(start)));
sign *= -1; // invert, since the upper-left corner of the canvas is at pixel (0, 0)
pixel = (reverse)
? me.top
: me.bottom;
}
if (realValue !== baseValue) {
if (baseValue === 0) { // include zero tick
offset = helpers.getValueOrDefault(
me.options.ticks.fontSize,
Chart.defaults.global.defaultFontSize
);
innerDimension -= offset;
range.start = firstTickValue;
}
if (realValue !== 0) {
offset += innerDimension / (lg(range.end) - lg(range.start)) * (lg(realValue) - lg(range.start));
}
pixel += sign * offset;
}
return pixel;
},
getValueForPixel: function(pixel) {
var me = this;
var range = helpers.log10(me.end) - helpers.log10(me.start);
var value, innerDimension;
var reverse = me.options.ticks.reverse;
var lg = helpers.log10;
var exp = Math.floor(lg(me.minNotZero));
var significand = Math.floor(me.minNotZero / Math.pow(10, exp));
var firstTickValue = significand * Math.pow(10, exp);
var range = (reverse)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, can we avoid nesting start and end under range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The minifier reduces the code to r=d?l.right:l.left, removing the parenthesis.

Being dyslexic, I have trouble reading lines like the following from the function determineDataLimits:
me.min = me.min === null ? minVal : Math.min(me.min, minVal);
For however short, every time it takes me about 10 seconds to understand what it does.
Expanding it over multiple lines and adding parenthesis removes this readability obstacle.

me.min = (me.min === null)
    ? minVal
    : Math.min(me.min, minVal);

Still it is just a personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

It's really minor and I don't have a strong opinion on which form is better (I usually go for the one which is consistent with the rest of the code if that makes sense).

Anyway, that wasn't my point for this line, but to flatten the useless range object:

var start = reverse ? me.end : me.start;
var end = reverse ? me.start : me.end;

? {start: me.end, end: me.start}
: {start: me.start, end: me.end};
var baseValue = range.start;
var innerDimension, value;

if (me.isHorizontal()) {
innerDimension = me.width;
value = me.start * Math.pow(10, (pixel - me.left) * range / innerDimension);
} else { // todo: if start === 0
value = (reverse)
? me.right - pixel
: pixel - me.left;
} else {
innerDimension = me.height;
value = Math.pow(10, (me.bottom - pixel) * range / innerDimension) / me.start;
value = (reverse)
? pixel - me.top
: me.bottom - pixel;
}
if (value !== baseValue) {
if (baseValue === 0) { // include zero tick
var offset = helpers.getValueOrDefault(
me.options.ticks.fontSize,
Chart.defaults.global.defaultFontSize
);
value -= offset;
innerDimension -= offset;
range.start = firstTickValue;
}
value *= lg(range.end) - lg(range.start);
value /= innerDimension;
value = Math.pow(10, lg(range.start) + value);
}
return value;
}
Expand Down
14 changes: 8 additions & 6 deletions test/specs/scale.logarithmic.tests.js
View file
Open in desktop
Expand Up @@ -761,9 +761,10 @@ describe('Logarithmic Scale tests', function() {
var yScale = chart.scales.yScale;
expect(yScale.getPixelForValue(70, 0, 0)).toBeCloseToPixel(32); // top + paddingTop
expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(484); // bottom - paddingBottom
expect(yScale.getPixelForValue(0.063, 0, 0)).toBeCloseToPixel(475); // minNotZero 2% from range
expect(yScale.getPixelForValue(0.5, 0, 0)).toBeCloseToPixel(344);
expect(yScale.getPixelForValue(4, 0, 0)).toBeCloseToPixel(213);
expect(yScale.getPixelForValue(0.06, 0, 0)).toBeCloseToPixel(472); // first tick fontsize from 0
expect(yScale.getPixelForValue(0.063, 0, 0)).toBeCloseToPixel(469); // minNotZero
expect(yScale.getPixelForValue(0.5, 0, 0)).toBeCloseToPixel(340);
expect(yScale.getPixelForValue(4, 0, 0)).toBeCloseToPixel(210);
expect(yScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(155);
expect(yScale.getPixelForValue(63, 0, 0)).toBeCloseToPixel(38.5);

Expand All @@ -772,9 +773,10 @@ describe('Logarithmic Scale tests', function() {

expect(yScale.getPixelForValue(70, 0, 0)).toBeCloseToPixel(484); // bottom - paddingBottom
expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(32); // top + paddingTop
expect(yScale.getPixelForValue(0.063, 0, 0)).toBeCloseToPixel(41); // minNotZero 2% from range
expect(yScale.getPixelForValue(0.5, 0, 0)).toBeCloseToPixel(172);
expect(yScale.getPixelForValue(4, 0, 0)).toBeCloseToPixel(303);
expect(yScale.getPixelForValue(0.06, 0, 0)).toBeCloseToPixel(44); // first tick fontsize from 0
expect(yScale.getPixelForValue(0.063, 0, 0)).toBeCloseToPixel(47); // minNotZero
expect(yScale.getPixelForValue(0.5, 0, 0)).toBeCloseToPixel(176);
expect(yScale.getPixelForValue(4, 0, 0)).toBeCloseToPixel(306);
expect(yScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(361);
expect(yScale.getPixelForValue(63, 0, 0)).toBeCloseToPixel(477);
});
Expand Down