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 #11300 #11484

Merged
merged 2 commits into from
Nov 8, 2019
Merged

Fix #11300 #11484

merged 2 commits into from
Nov 8, 2019

Conversation

susiwen8
Copy link
Contributor

For category type, one data show on window just check if scale contains it's value

Close #11300

@susiwen8
Copy link
Contributor Author

I got problem to install puppeteer, so this PR hasn't test with test cases.

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

Some personal thoughts. Waiting for @100pah or @pissang to give a further CR.

// For aixs type is category,
// only one data shows on viewport just check if it xAxis' value equal to scale
if (axisX.type === 'category' && axisX.scale.count() === 1) {
return data[0] === axisX.scale._extent[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

_extent is a private variable. You should probably call axisX.scale.getExtent()[0] to get it.

return data[0] === axisX.scale._extent[0];
}

return axisX.containData(data[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if you should fix the implementation of axisX.containData in the case of one data item.

@pissang
Copy link
Contributor

pissang commented Oct 25, 2019

Hi @susiwen8 .

I think the main cause is from https://github.com/apache/incubator-echarts/blob/master/src/coord/Axis.js#L88

Current solution of containData will convert the data to coordinates, then verify if coordinates is between the axis extent. Some information will be lost during this conversion. Like coord will be clamped as you metioned in #11300 (comment)

From my perspective, a better solution is using Scale#contain

containData: function (data) {
  return this.scale.contain(data);
}

BTW: You can try using cnpm to install puppeteer. It will use Taobao Mirror to download the executable.

@pissang pissang added this to the 4.6.0 milestone Oct 28, 2019
@susiwen8
Copy link
Contributor Author

@pissang Hi, I have tried this.scale.contain(data), for x value is fine, but not for y value. Because yAxis' extent only consider the data not the position of markpoint.

@pissang
Copy link
Contributor

pissang commented Nov 6, 2019

Hi @susiwen8 sorry for the late reply.

Because yAxis' extent only consider the data not the position of markpoint

I didn't understand this. Did you mean value type axis?

It will be great if you can give an example to explain it. I tried using this.scale.contain(data) and it works well on the example in the issue. I think it should make sense.

@susiwen8
Copy link
Contributor Author

susiwen8 commented Nov 6, 2019

Screen Shot 2019-11-06 at 13 28 47
Screen Shot 2019-11-06 at 13 28 57
Screen Shot 2019-11-06 at 13 29 07
Screen Shot 2019-11-06 at 13 29 12
@pissang Because the yAxis of markpoint for Aug and Dec are way higher than value, they aren't fit in Y axis if we use scale. Scale only consider data not markpoint.

@pissang
Copy link
Contributor

pissang commented Nov 6, 2019

Understand. I think it's another issue: If markPoint should be clipped if it exceed the axis. It should be a logic in mark point. Not in the underlying coordinate system.

But in this issue. Using this.scale.contain(data) would make more sense for the method containData.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants