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: minOpen is true will drop a piece #12147

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Fix: minOpen is true will drop a piece #12147

merged 2 commits into from
Feb 25, 2020

Conversation

susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented Feb 14, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

When minOpen is true, index shouldn't increase to 1

Fixed issues

Close #12121

Details

Before: What was the problem?

Screen Shot 2020-02-14 at 20 45 30

After: How is it fixed in this PR?

Screen Shot 2020-02-14 at 20 45 06

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Feb 14, 2020

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.

The pull request is marked to be PR: author is committer because you are a committer of this project.

pissang
pissang previously approved these changes Feb 16, 2020
@pissang
Copy link
Contributor

pissang commented Feb 16, 2020

In this way, the splitNumber is not 6 instead of 5. I'm not sure it's correct by design. @100pah Please have a look at this.

@pissang pissang requested review from 100pah and pissang February 16, 2020 09:19
@@ -402,7 +402,7 @@ var resetMethods = {

if (thisOption.minOpen) {
pieceList.push({
index: index++,
index: index,
Copy link
Member

Choose a reason for hiding this comment

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

Only remove ++ is not correct. Because after ++ removed, the first piece and the second piece has the same index.
That cause when click on the first piece, both the first piece and the second pieces are highlighted/downplayed.

Copy link
Member

@100pah 100pah Feb 16, 2020

Choose a reason for hiding this comment

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

Also consider
(1) @pissang mentioned: when the split number is 5, the final pieces should better be 5.
(2) The potential bug that the piece.index forget to change after reformIntervals(pieceList) called.

I think the code to resolve this issue could be:

    splitNumber: function () {
        var thisOption = this.option;
        var pieceList = this._pieceList;
        var precision = Math.min(thisOption.precision, 20);
        var dataExtent = this.getExtent();
        var splitNumber = thisOption.splitNumber;
        splitNumber = Math.max(parseInt(splitNumber, 10), 1);
        thisOption.splitNumber = splitNumber;
+       var closedIntervalCount = splitNumber - (+thisOption.minOpen) - (+thisOption.maxOpen);

-       var splitStep = (dataExtent[1] - dataExtent[0]) / splitNumber;
+       var splitStep = (dataExtent[1] - dataExtent[0]) / closedIntervalCount;
        // Precision auto-adaption
        while (+splitStep.toFixed(precision) !== splitStep && precision < 5) {
            precision++;
        }
        thisOption.precision = precision;
        splitStep = +splitStep.toFixed(precision);

-       var index = 0;

        if (thisOption.minOpen) {
            pieceList.push({
-               index++,
                interval: [-Infinity, dataExtent[0]],
                close: [0, 0]
            });
        }

        for (
-           var curr = dataExtent[0], len = index + splitNumber;
-           index < len;
-           curr += splitStep
+           var index = 0, curr = dataExtent[0];
+           index < closedIntervalCount;
+           curr += splitStep, index++
        ) {
            var max = index === splitNumber - 1 ? dataExtent[1] : (curr + splitStep);

            pieceList.push({
-               index: index++,
                interval: [curr, max],
                close: [1, 1]
            });
        }

        if (thisOption.maxOpen) {
            pieceList.push({
-               index: index++,
                interval: [dataExtent[1], Infinity],
                close: [0, 0]
            });
        }

        reformIntervals(pieceList);

-       zrUtil.each(pieceList, function (piece) {
+       zrUtil.each(pieceList, function (piece, index) {
+           piece.index = index;
            piece.text = this.formatValueText(piece.interval);
        }, this);
    },

@susiwen8
Copy link
Contributor Author

@pissang @100pah I have reservation on split number, clearly user wants to add a piece if they specify minOpen or maxOpen

I expect a piece to be added to the pieces in the chart, meaning that it should have 6 pieces now. 5 for the range between min and max value and 1 for values lower min.

@100pah
Copy link
Member

100pah commented Feb 25, 2020

@susiwen8

I have reservation on split number, clearly user wants to add a piece if they specify minOpen or maxOpen

@pissang have mentioned me and think your suggestion is reasonable:
That is,
finalPieceCount = userSpecifiedSplitNumber + (minOpen ? 1 : 0) + (maxOption ? 1 : 0)

@100pah 100pah merged commit 80d389f into apache:master Feb 25, 2020
@echarts-bot
Copy link

echarts-bot bot commented Feb 25, 2020

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

@susiwen8 susiwen8 deleted the #12121 branch February 25, 2020 07:54
@Ovilia Ovilia added this to the 4.8.0 milestone Feb 27, 2020
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.

visualMap - minOpen option breaks splitNumber and pieces
4 participants