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

docs(example): brush selection add the modification button example #4004

Merged
merged 5 commits into from
Jul 4, 2022
Merged

docs(example): brush selection add the modification button example #4004

merged 5 commits into from
Jul 4, 2022

Conversation

afetmin
Copy link
Contributor

@afetmin afetmin commented Jun 25, 2022

Features

用户可自定义按钮样式包括文字

image

Issues #3977

@hustcc
Copy link
Member

hustcc commented Jun 27, 2022

@afetmin 很赞,这么快就提交了 pr。但是在设计上有一些不太好,当前做法是在 Chart 上增加这些配置,然后交互 action 读取配置去展示,存在问题:

  1. 这个配置本身和 Chart 实例没有太大关系,理解上也不合适
  2. 交互中的一些 ui 都存在自定义的需求,如果这么做,Chart 上的配置项会爆炸掉

建议做法,registerAction 的时候,可以增加一些配置项。比如当前 Reset 这个ui 就是在这里注册的。

registerAction('reset-button', ButtonAction, {
  name: 'reset-button',
  text: 'reset',
});

所以只要我们重新 registerAction 这个 action,然后传入我们想要的配置是否就可以解决了?但是要重新 registerAction,我们需要拿到内置的 ButtonAction。

const ButtonAction = getActionClass('reset-button');

// 重新注册
registerAction('reset-button', ButtonAction, {
  name: 'reset-button',
  text: '重置',
});

如果这样可行的话,我们只需要将 getActionClass API 暴露出来即可。那这个 PR 只需要一行代码就可以完成。

@visiky
Copy link
Member

visiky commented Jun 27, 2022

不如考虑下,放在主题中;获取的时候,通过 view.getTheme() 获取到相关配置; 使用的时候如下:

theme: {
   // button: {...}
   // 考虑到这个交互已经命名局限为 reset-button 了,就直接主题中增加 resetButton 配置就好
   resetButton: {...}
}

@hustcc
Copy link
Member

hustcc commented Jun 27, 2022

不如考虑下,放在主题中;获取的时候,通过 view.getTheme() 获取到相关配置; 使用的时候如下:

theme: {
   // button: {...}
   // 考虑到这个交互已经命名局限为 reset-button 了,就直接主题中增加 resetButton 配置就好
   resetButton: {...}
}

长远来看,的确应该放在主题中,但是这个的的架构设计成本,有点太大,外部 PR 的同学来处理,有点难。上面的方案其实只需要将 getActionClass 报漏出来,业务开发自己来解决定制问题。

@visiky
Copy link
Member

visiky commented Jun 27, 2022

@afetmin 你可以考虑下 @hustcc 提供的方案

@afetmin
Copy link
Contributor Author

afetmin commented Jun 27, 2022

@afetmin 你可以考虑下 @hustcc 提供的方案
好的, 晚上看下

@coveralls
Copy link

coveralls commented Jun 27, 2022

Pull Request Test Coverage Report for Build 2576625487

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 91.276%

Totals Coverage Status
Change from base Build 2555046788: 0.01%
Covered Lines: 11820
Relevant Lines: 12458

💛 - Coveralls

@afetmin afetmin changed the title feat(button): users can customize the button style docs(brush): brush selection add the modification button example Jun 27, 2022
@afetmin
Copy link
Contributor Author

afetmin commented Jun 27, 2022

@afetmin 很赞,这么快就提交了 pr。但是在设计上有一些不太好,当前做法是在 Chart 上增加这些配置,然后交互 action 读取配置去展示,存在问题:

  1. 这个配置本身和 Chart 实例没有太大关系,理解上也不合适
  2. 交互中的一些 ui 都存在自定义的需求,如果这么做,Chart 上的配置项会爆炸掉

建议做法,registerAction 的时候,可以增加一些配置项。比如当前 Reset 这个ui 就是在这里注册的。

registerAction('reset-button', ButtonAction, {
  name: 'reset-button',
  text: 'reset',
});

所以只要我们重新 registerAction 这个 action,然后传入我们想要的配置是否就可以解决了?但是要重新 registerAction,我们需要拿到内置的 ButtonAction。

const ButtonAction = getActionClass('reset-button');

// 重新注册
registerAction('reset-button', ButtonAction, {
  name: 'reset-button',
  text: '重置',
});

如果这样可行的话,我们只需要将 getActionClass API 暴露出来即可。那这个 PR 只需要一行代码就可以完成。

功能已经暴露 💬

image
image

@@ -1,4 +1,4 @@
import { Chart } from '@antv/g2';
import { Chart, registerAction, getActionClass } from '@antv/g2';
Copy link
Member

Choose a reason for hiding this comment

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

居然现在已经导出 getActionClass 了,只需要新增一个 demo 即可。@afetmin 可以麻烦你改成新增 demo 吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hustcc 不知道这里的截图上传到哪?
"screenshot": "https://gw.alipayobjects.com/mdn/rms_f5c722/afts/img/A*PF0IS7OaCSwAAAAAAAAAAABkARQnAQ"

@afetmin afetmin changed the title docs(brush): brush selection add the modification button example docs(example): brush selection add the modification button example Jun 28, 2022
@visiky visiky merged commit de7425a into antvis:master Jul 4, 2022
@pr-triage pr-triage bot added the PR: merged label Jul 4, 2022
@github-actions
Copy link

github-actions bot commented Jul 4, 2022

🎊 PR Preview de7425a has been successfully built and deployed to https://antvis-G2-preview-pr-4004.surge.sh?type=diff&date=2022-07-04

🕐 Build time: 65.053s

🤖 By Surge Ui Insight

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.

None yet

4 participants