-
Notifications
You must be signed in to change notification settings - Fork 1.8k
WIP: feat: 迁移并优化v1动画能力 #4798
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
base: master
Are you sure you want to change the base?
WIP: feat: 迁移并优化v1动画能力 #4798
Conversation
Summary of ChangesHello @q32757468, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求迁移并优化了动画能力,为图表元素引入了新的动画方法。它提供了灵活的 SVG 动画工具,并将其集成到 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @q32757468, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求迁移并优化了图表库的动画能力,引入了一系列新的 SVG 动画功能,包括通用属性动画、变换动画和沿路径动画。这些增强功能旨在提升图表元素的视觉动态性和交互性,并提供了在边上动画显示令牌的特定功能,以更好地支持流程和状态的可视化。同时,新增了相应的示例来帮助开发者理解和使用这些新特性。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
这个 PR 很好地迁移并优化了 v1 的动画能力,通过在 CellView
和 EdgeView
上暴露新的动画方法,提供了更强大和灵活的动画功能。代码结构清晰,并且添加了丰富的示例来演示新功能。我发现了一些可以改进的地方:一处是 sendToken
方法中的拼写错误,另一处是 CellView
中动画相关方法的代码重复问题。修正这些问题将有助于提高代码的健壮性和可维护性。整体来说,这是一次很棒的更新。
let duration | ||
let reversed | ||
let selector | ||
let rorate | ||
let timing = 'linear' | ||
if (typeof options === 'object') { | ||
duration = options.duration | ||
reversed = options.reversed === true | ||
selector = options.selector | ||
if (options.rotate === false) { | ||
rorate = '' | ||
} else if (options.rotate === true) { | ||
rorate = 'auto' | ||
} else if (options.rotate != null) { | ||
rorate = `${options.rotate}` | ||
} | ||
|
||
if (options.timing) { | ||
timing = options.timing | ||
} | ||
} else { | ||
duration = options | ||
reversed = false | ||
selector = null | ||
} | ||
|
||
duration = duration || 1000 | ||
|
||
const attrs: AnimationOptions = { | ||
dur: `${duration}ms`, | ||
repeatCount: '1', | ||
calcMode: timing, | ||
fill: 'freeze', | ||
} | ||
|
||
if (rorate) { | ||
attrs.rotate = rorate | ||
} | ||
|
||
if (reversed) { | ||
attrs.keyPoints = '1;0' | ||
attrs.keyTimes = '0;1' | ||
} | ||
|
||
if (typeof options === 'object') { | ||
const { duration, reversed, selector, rotate, timing, ...others } = | ||
options | ||
Object.keys(others).forEach((key) => { | ||
attrs[key] = others[key] | ||
}) | ||
} |
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.
在 sendToken
方法中,变量 rorate
存在拼写错误,应为 rotate
。这个拼写错误导致 rotate
动画选项无法正确传递。请修正该变量及其所有引用。
let duration
let reversed
let selector
let rotate
let timing = 'linear'
if (typeof options === 'object') {
duration = options.duration
reversed = options.reversed === true
selector = options.selector
if (options.rotate === false) {
rotate = ''
} else if (options.rotate === true) {
rotate = 'auto'
} else if (options.rotate != null) {
rotate = `${options.rotate}`
}
if (options.timing) {
timing = options.timing
}
} else {
duration = options
reversed = false
selector = null
}
duration = duration || 1000
const attrs: AnimationOptions = {
dur: `${duration}ms`,
repeatCount: '1',
calcMode: timing,
fill: 'freeze',
}
if (rotate) {
attrs.rotate = rotate
}
if (reversed) {
attrs.keyPoints = '1;0'
attrs.keyTimes = '0;1'
}
if (typeof options === 'object') {
const { duration, reversed, selector, rotate, timing, ...others } =
options
Object.keys(others).forEach((key) => {
attrs[key] = others[key]
})
}
animate(elem: SVGElement | string, options: Util.AnimationOptions) { | ||
const target = typeof elem === 'string' ? this.findOne(elem) : elem | ||
if (target == null) { | ||
throw new Error('Invalid animation element.') | ||
} | ||
|
||
const revert = () => { | ||
if (!target.parentNode) { | ||
Dom.remove(target) | ||
} | ||
} | ||
|
||
if (!target.parentNode) { | ||
Dom.appendTo(target, this.graph.view.stage) | ||
} | ||
|
||
const onComplete = options.complete | ||
options.complete = (e: Event) => { | ||
revert() | ||
|
||
if (onComplete) { | ||
onComplete(e) | ||
} | ||
} | ||
|
||
return Util.animate(target as SVGElement, options) | ||
} | ||
|
||
animateTransform(elem: SVGElement | string, options: Util.AnimationOptions) { | ||
const target = typeof elem === 'string' ? this.findOne(elem) : elem | ||
if (target == null) { | ||
throw new Error('Invalid animation element.') | ||
} | ||
|
||
const revert = () => { | ||
if (!target.parentNode) { | ||
Dom.remove(target) | ||
} | ||
} | ||
|
||
if (!target.parentNode) { | ||
Dom.appendTo(target, this.graph.view.stage) | ||
} | ||
|
||
const onComplete = options.complete | ||
options.complete = (e: Event) => { | ||
revert() | ||
|
||
if (onComplete) { | ||
onComplete(e) | ||
} | ||
} | ||
|
||
return Util.animateTransform(target as SVGElement, options) | ||
} | ||
|
||
animateAlongPath( | ||
elem: SVGElement | string, | ||
options: { | ||
duration?: number | ||
reversed?: boolean | ||
selector?: string | ||
// https://developer.mozilla.org/zh-CN/docs/Web/SVG/Attribute/rotate | ||
rotate?: boolean | number | 'auto' | 'auto-reverse' | ||
// https://developer.mozilla.org/zh-CN/docs/Web/SVG/Attribute/calcMode | ||
timing?: 'linear' | 'discrete' | 'paced' | 'spline' | ||
} & Util.AnimationOptions, | ||
) { | ||
const target = typeof elem === 'string' ? this.findOne(elem) : elem | ||
if (target == null) { | ||
throw new Error('Invalid animation element.') | ||
} | ||
|
||
let path | ||
const { selector } = options | ||
|
||
if (typeof selector === 'string') { | ||
path = this.findOne(selector, this.container, this.selectors) | ||
} else { | ||
path = this.container.querySelector('path') | ||
} | ||
|
||
if (!(path instanceof SVGPathElement)) { | ||
throw new Error('Token animation requires a valid connection path.') | ||
} | ||
|
||
const revert = () => { | ||
if (!target.parentNode) { | ||
Dom.remove(target) | ||
} | ||
} | ||
|
||
if (!target.parentNode) { | ||
Dom.appendTo(target, this.graph.view.stage) | ||
} | ||
|
||
const onComplete = options.complete | ||
options.complete = (e: Event) => { | ||
revert() | ||
|
||
if (onComplete) { | ||
onComplete(e) | ||
} | ||
} | ||
|
||
return Util.animateAlongPath(target as SVGElement, options, path) | ||
} |
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.
animate
、animateTransform
和 animateAlongPath
方法中存在大量重复代码,主要用于准备动画目标元素(查找、附加到 DOM、包装 complete
回调)。为了提高代码的可维护性和减少冗余,建议将这部分通用逻辑提取到一个私有辅助方法中。
private _prepareAnimation(elem: SVGElement | string, options: Util.AnimationOptions) {
const target = typeof elem === 'string' ? this.findOne(elem) : elem
if (target == null) {
throw new Error('Invalid animation element.')
}
const revert = () => {
if (!target.parentNode) {
Dom.remove(target)
}
}
if (!target.parentNode) {
Dom.appendTo(target, this.graph.view.stage)
}
const onComplete = options.complete
options.complete = (e: Event) => {
revert()
if (onComplete) {
onComplete(e)
}
}
return target as SVGElement
}
animate(elem: SVGElement | string, options: Util.AnimationOptions) {
const target = this._prepareAnimation(elem, options)
return Util.animate(target, options)
}
animateTransform(elem: SVGElement | string, options: Util.AnimationOptions) {
const target = this._prepareAnimation(elem, options)
return Util.animateTransform(target, options)
}
animateAlongPath(
elem: SVGElement | string,
options: {
duration?: number
reversed?: boolean
selector?: string
// https://developer.mozilla.org/zh-CN/docs/Web/SVG/Attribute/rotate
rotate?: boolean | number | 'auto' | 'auto-reverse'
// https://developer.mozilla.org/zh-CN/docs/Web/SVG/Attribute/calcMode
timing?: 'linear' | 'discrete' | 'paced' | 'spline'
} & Util.AnimationOptions,
) {
const target = this._prepareAnimation(elem, options)
let path
const { selector } = options
if (typeof selector === 'string') {
path = this.findOne(selector, this.container, this.selectors)
} else {
path = this.container.querySelector('path')
}
if (!(path instanceof SVGPathElement)) {
throw new Error('Token animation requires a valid connection path.')
}
return Util.animateAlongPath(target, options, path)
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4798 +/- ##
==========================================
- Coverage 82.23% 81.55% -0.69%
==========================================
Files 328 327 -1
Lines 37613 37869 +256
Branches 7584 7591 +7
==========================================
- Hits 30930 30883 -47
- Misses 6659 6962 +303
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Code Review
这次的 PR 迁移并优化了 v1 的动画能力,引入了 animate
、animateTransform
、animateAlongPath
和 sendToken
等新的动画功能,并提供了相应的示例,功能强大。实现基于 SVG SMIL,这是一个很好的选择。我的审查主要关注于提高代码质量、可维护性和健壮性。我发现了一些代码重复、可以更清晰实现的逻辑以及可以改进的错误处理等方面的问题。
} catch (error) { | ||
// pass | ||
} |
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.
animate(elem: SVGElement | string, options: Util.AnimationOptions) { | ||
const target = typeof elem === 'string' ? this.findOne(elem) : elem | ||
if (target == null) { | ||
throw new Error('Invalid animation element.') | ||
} | ||
|
||
const revert = () => { | ||
if (!target.parentNode) { | ||
Dom.remove(target) | ||
} | ||
} | ||
|
||
if (!target.parentNode) { | ||
Dom.appendTo(target, this.graph.view.stage) | ||
} | ||
|
||
const onComplete = options.complete | ||
options.complete = (e: Event) => { | ||
revert() | ||
|
||
if (onComplete) { | ||
onComplete(e) | ||
} | ||
} | ||
|
||
return Util.animate(target as SVGElement, options) | ||
} | ||
|
||
animateTransform(elem: SVGElement | string, options: Util.AnimationOptions) { | ||
const target = typeof elem === 'string' ? this.findOne(elem) : elem | ||
if (target == null) { | ||
throw new Error('Invalid animation element.') | ||
} | ||
|
||
const revert = () => { | ||
if (!target.parentNode) { | ||
Dom.remove(target) | ||
} | ||
} | ||
|
||
if (!target.parentNode) { | ||
Dom.appendTo(target, this.graph.view.stage) | ||
} | ||
|
||
const onComplete = options.complete | ||
options.complete = (e: Event) => { | ||
revert() | ||
|
||
if (onComplete) { | ||
onComplete(e) | ||
} | ||
} | ||
|
||
return Util.animateTransform(target as SVGElement, options) | ||
} |
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.
animate
和 animateTransform
方法的实现几乎完全相同,唯一的区别是它们分别调用了 Util.animate
和 Util.animateTransform
。这造成了代码重复。建议将通用逻辑提取到一个私有方法中,以提高代码的可维护性。
private createCellAnimation(
elem: SVGElement | string,
options: Util.AnimationOptions,
animateFn: (elem: SVGElement, options: Util.AnimationOptions) => () => void,
) {
const target = typeof elem === 'string' ? this.findOne(elem) : elem
if (target == null) {
throw new Error('Invalid animation element.')
}
const revert = () => {
if (!target.parentNode) {
Dom.remove(target)
}
}
if (!target.parentNode) {
Dom.appendTo(target, this.graph.view.stage)
}
const onComplete = options.complete
options.complete = (e: Event) => {
revert()
if (onComplete) {
onComplete(e)
}
}
return animateFn(target as SVGElement, options)
}
animate(elem: SVGElement | string, options: Util.AnimationOptions) {
return this.createCellAnimation(elem, options, Util.animate)
}
animateTransform(elem: SVGElement | string, options: Util.AnimationOptions) {
return this.createCellAnimation(elem, options, Util.animateTransform)
}
if (typeof selector === 'string') { | ||
path = this.findOne(selector, this.container, this.selectors) | ||
} else { | ||
path = this.container.querySelector('path') |
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.
src/view/edge/index.ts
Outdated
let duration | ||
let reversed | ||
let selector | ||
let rorate |
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.
if (typeof selector === 'string') { | ||
path = this.findOne(selector, this.container, this.selectors) | ||
} else { | ||
path = this.container.querySelector('path') |
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.
}) | ||
const view2 = graph.findView(rect2) | ||
|
||
view2?.on('view:render', () => { |
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.
用法上感觉怪怪的
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.
我感觉也和现有的其他使用形式不太搭,但是考虑和参考了很多其他的方式,只有这种改动成本最低最使用最简单了,开会可以再讨论讨论~
// pass | ||
} | ||
|
||
return () => {} |
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.
是否考虑支持链式调用 .animate().animate()
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.
好想法,从使用形式上来看支持链式调用在添加多个动画效果时会方便很多,尝试支持一下看看支持链式调用会不会有其他问题
bbf7032
to
0dcf0d4
Compare
const token = Vector.create('circle', { r: 6, fill: 'green' }) | ||
|
||
view.on('view:render', () => { | ||
view.sendToken(token.node, { |
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.
这个示例感觉也不合适。
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.
可考虑下配置式实现,默认是节点加载完成开始动画,节点上添加对应的 API,例如 startAnimate
、 stopAnimate
0dcf0d4
to
9d1b81f
Compare
Description
不同动画api的效果图:



Motivation and Context
Types of changes
Self Check before Merge