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

feat(event): add plot events bind #1412

Merged
merged 3 commits into from
Aug 10, 2020
Merged

feat(event): add plot events bind #1412

merged 3 commits into from
Aug 10, 2020

Conversation

hustcc
Copy link
Member

@hustcc hustcc commented Aug 10, 2020

方案有二个

  • 属性配置的方式
const line = new Line({
   event: {
      'element:click': () => {},
   }
});

优点:保持兼容性,完全配置化
缺点:代码相对复杂,需要去 diff events 配置,然后做 bind 和 unbind(echarts 做 react 包装的时候遇到这个问题)

  • API 的方式
const line = new Line({ /*...*/ });

line.on('element:click', () => {});

优点:代码简单,完全继承 G2 能力(echarts 使用这个方式)
缺点:和 v1 不兼容

大家看看哪种比较好,或者还有其他的方面没有考虑到


  • 为 plot 提供事件机制(目前是 API 方式)
  • 单测

@auto-add-label auto-add-label bot added the enhancement New feature or request label Aug 10, 2020
Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Could you please add tests to make sure this change works as expected?

private bindEvents() {
if (this.chart) {
this.chart.on('*', (e: Event) => {
if (e?.type) {
Copy link
Member

Choose a reason for hiding this comment

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

if (e?.type && contains(keys(this.getEvents()), e?.type)) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

就是每个图表支持哪些 event 有图表决定吗?这样是在 G2 的事件能力上做了限制了~

Copy link
Member

Choose a reason for hiding this comment

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

直接透传就行了

Copy link
Member

Choose a reason for hiding this comment

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

是的,取决于图表注册了哪些事件,例如 chart.on('line:click',(e)=> console.log(e));

Copy link
Member Author

@hustcc hustcc Aug 10, 2020

Choose a reason for hiding this comment

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

然后 line:click 映射到 G2 element:click,但看这一个图表是容易理解了,但是从开发者来看:

  1. G2Plot 提供的是通用图表,对于非通用图表,还是会基于 G2,那么这个时候他们可以对于对事件就有两套命名认知了。这是提升了整体成本的
  2. G2, G2Plot 事件文档两份。
  3. 而且 G2 的事件通过 xx: yy 叉乘出上百种事件,G2Plot 这层维护映射关系太难了,而且后很容易出现变量命名上的困难的争论,导致 break change。

比如 G2 中 lable:click,如果在 G2Plot 2.0.0 没有映射掉,如果后面做映射成 pie-label:click 那么就会产生 breakchange,需要发大版本(3.0.0),或者两种命名都支持,上百个事件都存在这类困扰,维护起来也是麻烦。


function click(): Promise<Event> {
return new Promise((resolve, reject) => {
line.on('element:click', (e) => {
Copy link
Member

Choose a reason for hiding this comment

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

element:click 做个映射?

Copy link
Member Author

Choose a reason for hiding this comment

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

映射是改事件名称吗?如果 elemnt:click 改成 line:click 可能但这个图来看更语意了,但是就在 G2 的基础增加新概念了。

Copy link
Member

Choose a reason for hiding this comment

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

感觉 element:click 不够明确。

Copy link
Member

Choose a reason for hiding this comment

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

考虑用户学习成本,同时学习 G2、G2Plot 概念太多,对于他们后续想要做定制也有理解成本。element 其实很明确了,就是图形元素,这些都可以用一个基础概念教程介绍下就行了

@lxfu1 lxfu1 merged commit 4074bba into v2 Aug 10, 2020
@lxfu1 lxfu1 deleted the feat-plot-events branch August 10, 2020 07:36
BBSQQ pushed a commit that referenced this pull request Aug 25, 2020
* feat(event): add plot events bind

* test(event): add test cases & type define

* fix(core): fix by prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants