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: #8962 闭合多边形时，如果第一个值为NaN，导致最后值也为NaN，渲染不出连接线。 #9162

Merged
merged 2 commits into from
Oct 15, 2018
Merged

fix: #8962 闭合多边形时，如果第一个值为NaN，导致最后值也为NaN，渲染不出连接线。 #9162

merged 2 commits into from
Oct 15, 2018

Conversation

usagiring
Copy link
Contributor

@usagiring usagiring commented Oct 3, 2018 •
edited
Loading

另外：如果data中包含非Number类型，animation 过程中移动鼠标，导致连接线消失

@usagiring usagiring closed this Oct 3, 2018
@usagiring usagiring reopened this Oct 3, 2018
Ovilia
Ovilia requested changes Oct 8, 2018
View reviewed changes
src/chart/radar/radarLayout.js Outdated
@@ -38,7 +38,12 @@ export default function (ecModel) {

data.each(function (idx) {
// Close polygon
points[idx][0] && points[idx].push(points[idx][0].slice());

let index = points[idx].findIndex(([x, y]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

不要用 ES6 的语法，跟 ECharts 项目保持一致吧。所以 let、findIndex、箭头表达式就不要用了。

src/chart/radar/radarLayout.js Outdated
return !isNaN(x) && !isNaN(y)
})

index >= 0 && points[idx].push(points[idx][index].slice());
Copy link
Contributor

Choose a reason for hiding this comment

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

增加些英文注释，这个修改什么意思？

Ovilia
Ovilia requested changes Oct 11, 2018
View reviewed changes
Copy link
Contributor

@Ovilia Ovilia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very close now! 😄
Please undo the changes under dist.
You may write some meaningful comments for each commit, instead of the same one.

src/chart/radar/radarLayout.js Outdated
@@ -38,8 +39,19 @@ export default function (ecModel) {

data.each(function (idx) {
// Close polygon
points[idx][0] && points[idx].push(points[idx][0].slice());

var _idx = findFirstNotNaNPoint()
Copy link
Contributor

Choose a reason for hiding this comment

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

ECharts use camel cases, and it's better to name it something meaningful, like firstId.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add ; at the end of each line. Also note other places.

src/chart/radar/radarLayout.js Outdated
data.setItemLayout(idx, points[idx]);

function findFirstNotNaNPoint(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be function findFirstNotNaNPoint() {

src/chart/radar/radarLayout.js Outdated
data.setItemLayout(idx, points[idx]);

function findFirstNotNaNPoint(){
var _value = zrUtil.find(points[idx], function (point) {
Copy link
Contributor

Choose a reason for hiding this comment

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

value instead of _value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank for your very nice teach! I'm really new for git collaboration. I rebased my commits.
but maybe we need some contributing guidelines?
I didn't actually thing, if I pass NaN value to render, connect line often disappear when I move mouse or hover point. i dont know what cause the bug yet. I will try to do it

ps: very like your video , i can't wait for next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but should we pass a NaN value to render when we not need a value (such as type "-")?

Ovilia
Ovilia requested changes Oct 15, 2018
View reviewed changes
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.

One more change needed, and it will be merged! 😄

src/chart/radar/radarLayout.js Outdated
function findFirstActualPoint() {
return zrUtil.find(points[idx], function (point) {
return !isNaN(point[0]) && !isNaN(point[1]);
}) || ["NaN", "NaN"];
Copy link
Contributor

Choose a reason for hiding this comment

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

" shoud be '.

@Ovilia Ovilia merged commit 5cfd508 into apache:master Oct 15, 2018
@Ovilia
Copy link
Contributor

Ovilia commented Oct 15, 2018

Thanks for your contribution!

Now you are one of the ECharts contributors. Since we joined Apache group, you need to assign ICLA for your first PR.
Please fill in the PDF and print it, then sign on it and send the scanned file to secretary@apache.org and oviliazhang@gmail.com mentioning this is the ICLA file for project incubator-echarts.
This may be a little tricky, and sorry for the trouble. This is required for the first time your commit is merged in. If you refused, your commit will be backed off.

And are you interested in being an ECharts committer? This has more restrict rules than contributors. Committers are those who contribute more frequently and play more important roles on ECharts project.

After being a committer:

  • You can have write access to the ECharts code
  • You can mange issues (add tags, assign to someone, ...)
  • You can discuss about features with us committers more frequently with Email
  • You name and personal page link will be displayed at ECharts Website About page
  • See this for more information

If you are interested, here's what you can do:

  • Subscribe our mailing list by send an email to dev-subscribe@echarts.apache.org
  • When you want to discuss with us about something, send email to dev@echarts.apache.org
  • Help others by replying issues regularly
  • Make more useful pull requests (fix bugs or make enhancement in issues)

And you will be invited to be a committer when we think your contribution and knowledge of ECharts project is enough. We are looking forward to this! 😃

@usagiring
Copy link
Contributor Author

You can mange issues (add tags, assign to someone, ...)

…… mange -> manage?

ok.It's so cool ! I will send recently. but I don't understand the meaning of optional blank filling . What is it used for ?

Thanks for very nice teach! (催更)

@Ovilia
Copy link
Contributor

Ovilia commented Oct 16, 2018

What is blank filling?

@usagiring
Copy link
Contributor Author

sorry... 😭
Such as Public name / preferred Apache id(s) / notify project (optional)
that's something wrong if I not filling in ?

@Ovilia
Copy link
Contributor

Ovilia commented Oct 17, 2018

Never mind. You will get informed if anything goes wrong.
So the secretary not replied yet? We have another new contributor sending ICLA file and got no reply. I've just sent an email to our mentor about this.

@snyk-bot snyk-bot mentioned this pull request Dec 5, 2019
Open
This was referenced Mar 24, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ovilia Ovilia Ovilia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@usagiring @Ovilia