-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(tooltip): add 'datum[]' param to the enable function #6637
Conversation
0d932c6
to
19098a3
Compare
@TZZack PR 很赞,代码也挺工整,可以帮忙尝试加一个单侧:
|
嗯嗯,好的 |
d162dd1
to
2ce9c72
Compare
|
可以尝试不生成截图,不用 单测的目的是给一些逻辑加一把锁,下次修改这里的逻辑的时候,如果带来不兼容,能通过单测报错来报警。 |
2ce9c72
to
99c2c36
Compare
ok了哈,感谢指导! |
@TZZack 对于第1个修改点,我们对待参数添加的变化比较慎重,对于你提到的问题,你可以通过以下方式获取 Graph 实例并调用 API 获取相应数据: const graph = new Graph({
// ...
plugins: [
function () {
// 此处 this 为 graph
const graph = this;
return {
type: 'tooltip',
};
},
],
}); 对于第 2 个修改点,我认为是合理的,你可以仅针对该项内容提交相关代码。 |
@Aarebecca 第一个点是根据收集到的 tooltip 数据来判断是否显示 tooltip 吧?用 graph 怎么判断? |
@Aarebecca @hustcc 两位大佬,我是这样想的,感觉这个参数加上去的改动是可控的,因为这里逻辑并不复杂,而且这样也跟getContent的参数保持一致、以及对使用体验有一定的提升 |
Situation
Tooltip
plugin has no datum to do theenable
judgement ( enable function has just aevent
param). Maybe we can useevent.target.context.graph.getNodeData
andevent.target.id
to get datum, butevent.target.context
is aprivate
which will causets
error.Optimization
In this situation, I will add
datum[]
param toenable
function which is same as thegetContent
function.Besides, I want to improve the
show
logic that ifgetContent
returns nothing,tooltip
will not show.