-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat: add Repo Activity Racing Bar #696
feat: add Repo Activity Racing Bar #696
Conversation
The function of the main body has been initially realized, and the following is a demonstration video. I have not modified the text on the right. 2023-07-09.21-22-46.mp4 |
Hey @andyhuang18, the animation in your demo is very laggy, do you know why? |
I changed the |
I have successfully completed the development of this feature, and the |
Nicely done! I just tested your work ant it is amazing! It moves smoothly and looks awesome. Here is some advice on this:
Again, thanks for your incredible work! 👍 |
Hi~ @wxharry .Thanks for the great suggestions! I have modified the styles of the chart and |
Hey @andyhuang18, thanks for your hard work! oneI pulled your code and tested it with my hands, finding that the feature occasionally cannot be displayed with the following error output in the console: I have not found a way to replicate the bug yet. But I guess it is possibly related to feature loading timing. two
Please give the Replay button a new home: The displacement of the button may require you to learn and use more skills in HTML, CSS, React. Good luck :) threeI have not read the code changes yet. I assume they will be reviewed in days. |
Hi @andyhuang18, just change the to "feat: add Repo Activity Racing Bar". You are not just implementing this, you are adding the entire feature in this pr. 😄 |
feat: initial implementation of activity racing bar(need to be modified) feat: initial implementation of activity racing bar(need to be modified v1) feat: initial implementation of activity racing bar(need to be modified v2) feat: implementation of activity racing bar feat: change the style of chart and button
804e2a7
to
7aedbc3
Compare
Hi, @andyhuang18 . There are two style changes required for this PR:
Thanks for your contributions and feel free to leave comments or dm me~ |
OK! Thanks harry for the suggestion! I am modifying my code. Commit can be done today. 😆 |
Hi~ @tyn1998 @wxharry I've done all the development of this feature so far.
PS: I didn't know enough about the update mode of echart before, so If you have any additional suggestions, please let me know, thank you~ |
Hey! Thanks again for your contributions. This feature is really awesome! I have reviewed your code and left a comment. It won't take long to resolve and I think this feature is ready to merge. I am so excited to see the racing bar released. |
Hi @wxharry. Where is the problem? Seems like I didn't see your comment. 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andyhuang18, I have pushed several commits to improve the code.
Your contribution is great, thank you very much!
// clear timer if user replay the chart before it finishes | ||
if (timer) { | ||
clearTimeout(timer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timer should be cleared.
const playNext = () => { | ||
updateMonth(instance, data, months[i]); | ||
i++; | ||
if (i < months.length) { | ||
timer = setTimeout(playNext, updateFrequency); | ||
} | ||
}; | ||
|
||
playNext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another way to play
// @ts-ignore | ||
option.yAxis.axisLabel.rich = rich; | ||
// @ts-ignore | ||
option.series[0].data = data[month]; | ||
// @ts-ignore | ||
option.graphic.elements[0].style.text = month; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// @ts-ignore
is like xxx: any
, we should fix them all one day :D
// it seems that hidden bars are also rendered, so when each setOption merge more data into the chart, | ||
// the fps goes down. So we use notMerge to avoid merging data. But this disables the xAxis animation. | ||
// Hope we can find a better solution. | ||
instance.setOption(option, { | ||
notMerge: true, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wxharry, please take some time to review my recent commits to this branch. If you think the PR is ready to go, just |
+1 |
})(i); | ||
} | ||
|
||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there are many directive in this part, I think it would be better if we could remove some of these. The @ts-ignore
could be convenient but there are too many of them which may ignore type errors. Please confirm they are necessary. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, @ts-ignore
and any
should be avoided with best effort. In my commits, I removed some of them, but still left some others. My time is limited🤣 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have Andy fixing this later as a patch. I will merge this feature anyway but you can still improve your work. @andyhuang18 What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There is an inconsistence when open perceptor tab in different repos. For our repo, the racing bar is the first section, but in repos like vscode, it is the last one. It is not strongly related to this feature, so I will create a new issue. This feature looks good to me. Thanks @andyhuang18 @tyn1998 for your contributions! /approve |
Brief Information
This pull request is in the type of (more info about types):
Related issues (all available keywords):
Details
I have laid a foundation for this feature, which can be viewed in the option list. But the actual functionality has not yet been implemented. I'll follow up as soon as I have time!
Checklist
Others