-
Notifications
You must be signed in to change notification settings - Fork 651
feat: 增加布局组件 #2055
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?
feat: 增加布局组件 #2055
Conversation
a38aafc
to
6d6b502
Compare
Log
|
/gemini review |
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 增加了一个强大的 Layout
布局组件,支持多种布局模式,这是一个很棒的功能。代码结构清晰,并且包含了相应的测试用例。
我发现了一些潜在的问题,主要集中在布局计算逻辑上,可能会导致在某些场景下布局不正确。其中最主要的问题是手动计算的定位坐标没有被应用到子元素上。此外,栅格布局的计算方式在子元素高度不一时会失效。测试代码中存在过长的延时,可能会影响测试效率。具体细节请看我的评论。修复这些问题后,这个组件会更加健壮和可靠。
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<group | ||
style={{ | ||
width, | ||
height, | ||
...itemStyle, | ||
}} | ||
> | ||
{child} | ||
</group> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left = containerWidth / 2 + radius * Math.cos(angle); | ||
top = containerHeight / 2 + radius * Math.sin(angle); |
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 canvas = new Canvas(props); | ||
await canvas.render(); | ||
|
||
await delay(1000); |
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.
Checklist
npm test
passesDescription of change