Closed
Conversation
tangying1027
force-pushed
the
feat-multi-charts
branch
from
a38aafc to
6d6b502
Compare
LogDetails |
Contributor
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Comment on lines
+107
to
+112
| if (tag === Shape) { | ||
| if (type === 'circular') { | ||
| return <group style={{ x: left, y: top, ...itemStyle }}>{child}</group>; | ||
| } | ||
| return child; | ||
| } |
There was a problem hiding this comment.
对于 Shape 类型的子元素，只有在 circular 布局下才应用了定位。在 horizontal, vertical, grid 布局下，计算出的 left 和 top 值被忽略了，Shape 元素会直接返回，导致它们堆叠在原点。应该为所有 Shape 元素统一应用定位。
if (tag === Shape) {
return <group style={{ x: left, y: top, ...itemStyle }}>{child}</group>;
}
Comment on lines
+115
to
+123
| <group | ||
| style={{ | ||
| width, | ||
| height, | ||
| ...itemStyle, | ||
| }} | ||
| > | ||
| {child} | ||
| </group> |
There was a problem hiding this comment.
left 和 top 定位属性没有应用到子组件上。这里计算出的 left 和 top 值没有在包裹的 <group> 组件的 style 中使用，导致子组件的位置不正确。
另外，在 getLayoutStyle 方法中使用了 display: 'flex'，这与手动计算 left/top 的绝对定位方式存在冲突。建议统一采用一种布局方式。如果采用手动定位，应从 getLayoutStyle 中移除 flex 相关属性，并在这里正确应用 x 和 y 坐标。
<group
style={{
x: left,
y: top,
width,
height,
...itemStyle,
}}
>
{child}
</group>
| const itemWidth = (containerWidth - totalGapWidth) / columns; | ||
| width = itemWidth; | ||
| left = col * (itemWidth + horizontalGap); | ||
| top = row * (height + verticalGap); |
There was a problem hiding this comment.
grid 布局的行定位计算方式存在问题。这里的 top 值是基于 height 计算的，而 height 来自子元素自身的样式，如果子元素没有设置高度，则会取容器的高度。这在栅格布局中会导致问题：
- 如果子元素没有高度，
height会是containerHeight，这通常是不对的。 - 如果子元素高度不同，下一行的起始位置会基于前一个元素的高度计算，导致同行元素无法对齐。
建议考虑为 grid 布局增加一个 itemHeight 属性，或者动态计算每行的最大高度来确定下一行的偏移量。
Comment on lines
+102
to
+103
| left = containerWidth / 2 + radius * Math.cos(angle); | ||
| top = containerHeight / 2 + radius * Math.sin(angle); |
There was a problem hiding this comment.
circular 布局中，子元素是根据其左上角进行定位的。这意味着子元素会以其左上角顶点分布在圆周上。为了视觉上更居中，可以考虑在计算 left 和 top 时减去子元素自身宽高的一半。例如 left = ... - childWidth / 2。当然，获取子元素的宽高可能需要一些额外的工作。
| const canvas = new Canvas(props); | ||
| await canvas.render(); | ||
|
|
||
| await delay(1000); |
There was a problem hiding this comment.
在测试中使用 delay(1000) 等待 1 秒可能没有必要，并且会显著拖慢整个测试套件的执行速度。delay 函数内部已经使用了 setTimeout 和 requestAnimationFrame 来确保渲染完成。可以尝试一个更短的延迟，例如 delay(100) 或者更小的值，只要能保证快照正确即可。
Suggested change
| await delay(1000); | |
| await delay(100); |
Contributor
Author
|
没想好 下次吧 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
npm testpassesDescription of change