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

fix(OrdinalScale): fix line graph renders null value incorrectly. clo… #16672

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

WindyZ99
Copy link
Contributor

…se #16664

Brief Information

This pull request is in the type of: bug fixing

What does this PR do?

Fix function OrdinalScale.parse returns an unexpected value 0 with null.

Fixed issues

Details

Before: What was the problem?

When options like this:

options = {
        animation: false,
        xAxis: {
            type: "category",
            splitLine: { show: true },
            data: ["1:00", "2:00", "3:00", "4:00", "5:00"]
        },
        yAxis: {
            type: "category",
            axisLabel: {
            show: true
            },
            showMinLabel: true,
            showMaxLabel: true,
            data: ["A", "B", "C"]
        },
        series: [
            {
            type: "line",
            data: ["null", "A", undefined, "C", null], // with value `null`
            symbol: "circle",
            showSymbol: true,
            symbolSize: 8,
            lineStyle: {
                opacity: 0
            },
            connectNulls: false
            }
        ]
    };

We expect the value of null will not be rendered,
but ECharts renders it like the following:
image

When OrdinalScale.parse function handle the input value of null,
it returns 0 rather than NaN,
because Math.round(null) will produce 0.
We expect null will behave the same as undefined.

So I add a judgement in OrdinalScale.parse.

parse(val: OrdinalRawValue | OrdinalNumber): OrdinalNumber {
        // Caution: Math.round(null) will return `0` rather than `NaN`
        if (val == null) {
            val = undefined;
        }
        return isString(val)
            ? this._ordinalMeta.getOrdinal(val)
            // val might be float.
            : Math.round(val);
    }

After: How is it fixed in this PR?

image

@echarts-bot
Copy link

echarts-bot bot commented Mar 13, 2022

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

@@ -130,6 +130,10 @@ class OrdinalScale extends Scale<OrdinalScaleSetting> {
}

parse(val: OrdinalRawValue | OrdinalNumber): OrdinalNumber {
// Caution: Math.round(null) will return `0` rather than `NaN`
if (val == null) {
val = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can return NaN directly here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't realize this.

@pissang pissang added this to the 5.3.2 milestone Mar 14, 2022
@susiwen8
Copy link
Contributor

susiwen8 commented Mar 14, 2022

@pissang pissang merged commit 3cb8cea into apache:master Mar 15, 2022
@echarts-bot
Copy link

echarts-bot bot commented Mar 15, 2022

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@WindyZ99 WindyZ99 deleted the fix-16664 branch March 15, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants