Improve radial scale

[etimberg]

A lot of cleanup of the radial scale. I moved a lot of functionality into private functions.

Issues Fixed

• When the scale was user for the polar area graph, the scale didn't use the full available area because it tried to fit the point labels (used for radar chart) even though non existed.
• The point labels can now have multiple lines of text. Resolves Line Breaks on Radar Charts not working #3225

Testing Done

• Unit tests run
• Tested the polar area and radar charts manually

Changed Behaviour

• the getIndexAngle method no longer subtracts 90 degrees since we added it back almost all of the time.

[simonbrunel]

Some code enhancement suggestions (not tested)

[simonbrunel]

if (helpers.isArray(label)) {
    return {
        w: ...
        h: ...
    };
}

return {
  w: ...
  h: ...
};

[simonbrunel]

Could be normalized with multiple var declarations and grouping uninitialized vars at the end.

[simonbrunel]

Why a function?

[etimberg]

That was to make eslint happy otherwise the function is too long and too complex

[simonbrunel]

Not sure what the benefit to have functions for determineXLimits, determineYLimits and updateLimits, they are called only from that location. determineXLimits and determineYLimits could be refactored and updateLimits inlined:

function determineLimits(angle, pos, size, min, max) {
    if (angle == min || angle == max) {
        return {
            start: pos - (size / 2),
            end: pos + (size / 2)
        };
    }
    if (angle < min || angle > max) {
        return {
            start: pos - size - 5,
            end: pos
        };
    }
    return {
        start: pos,
        end: pos + size + 5
    };
}

var valueCount = getValueCount(scale);
for (i = 0; i < valueCount; i++) {
    // ... previous code ...
    var hlimits = determineLimits(angle, pointPosition.x, textSize.w, 0, 180);
    var vlimits = determineLimits(angle, pointPosition.y, textSize.h, 90, 270);

    // update limits
    if (furthestLeft > hlimit.start) {
        furthestLeft = hlimit.start;
        furthestLeftAngle = angleRadians;
    }
    if (furthestRight < hlimit.end) {
        furthestRight = hlimit.end;
        furthestRightAngle = angleRadians;
    }
    // ... rest of the updateLimits method ...
}

[etimberg]

Those functions came to make eslint happy otherwise it complains that fitWithPointLabels is too complex and has too many statements

[simonbrunel]

I don't think inner functions are a good approach to reduce code complexity, seems just a hack to trick eslint, but that makes the code more complex since these inner methods modify outer variables (also not good for performance to call useless functions in a loop).

determineLimits doesn't refer to outer scope variables, so can live outside fitWithPointLabels.

[simonbrunel]

if (angle === 0 || angle === 180) {
    return 'center';
}
if (angle < 180) {
    return  'left';
}
return 'right';

[simonbrunel]

Let's avoid empty path drawing and i === 0 condition in the loop:

var count = getValueCount(scale);
if (count === 0) {
    return;
}

ctx.beginPath();

var pos = scale.getPointPosition(0, radius);
ctx.moveTo(pos.x, pos.y);

for (var i = 1; i < count; i++) {
    pos = scale.getPointPosition(i, radius);
    ctx.lineTo(pos.x, pos.y);
}

ctx.closePath();
ctx.stroke();

[simonbrunel]

Quite personal, but I'm not a huge fan of long variable names, especially when the scope is obvious: it makes the code harder to read.

function getPointLabelFontOptions(scale) {
    var options = scale.options.pointLabels;
    var size = helpers.getValueOrDefault(options.fontSize, globalDefaults.defaultFontSize);
    var style = helpers.getValueOrDefault(options.fontStyle, globalDefaults.defaultFontStyle);
    var family = helpers.getValueOrDefault(options.fontFamily, globalDefaults.defaultFontFamily);

    return {
        size: size,
        style: style,
        family: family,
        font: helpers.fontString(size, style, family);
    };

[etimberg]

@simonbrunel I agree with your comments. i will make updates to the code to fix them

[etimberg]

@chartjs/maintainers is this good to go?

[simonbrunel]

Great work!