Skip to content

Conversation

tangying1027
Copy link
Contributor

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Description of change

Copy link

Fails
🚫

node failed.

Log

Error:  RequestError [HttpError]: Artifact has expired
    at /home/runner/work/F2/F2/node_modules/@octokit/core/node_modules/@octokit/request/dist-node/index.js:122:21
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async dangerfile.js:32:24 {
  status: 410,
  response: {
    url: 'https://api.github.com/repos/antvis/F2/actions/artifacts/1685653157/zip',
    status: 410,
    headers: {
      'access-control-allow-origin': '*',
      'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset',
      'content-length': '139',
      'content-security-policy': "default-src 'none'",
      'content-type': 'application/json; charset=utf-8',
      date: 'Wed, 30 Apr 2025 09:25:25 GMT',
      'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
      server: 'github.com',
      'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
      vary: 'Accept-Encoding, Accept, X-Requested-With',
      'x-accepted-oauth-scopes': '',
      'x-content-type-options': 'nosniff',
      'x-frame-options': 'deny',
      'x-github-api-version-selected': '2022-11-28',
      'x-github-media-type': 'github.v3; format=json',
      'x-github-request-id': 'C407:F7196:39892C:72DB45:6811EC85',
      'x-oauth-scopes': 'repo, write:packages',
      'x-ratelimit-limit': '5000',
      'x-ratelimit-remaining': '4968',
      'x-ratelimit-reset': '1746006223',
      'x-ratelimit-resource': 'core',
      'x-ratelimit-used': '32',
      'x-xss-protection': '0'
    },
    data: {
      message: 'Artifact has expired',
      documentation_url: 'https://docs.github.com/rest/actions/artifacts#download-an-artifact',
      status: '410'
    }
  },
  request: {
    method: 'GET',
    url: 'https://api.github.com/repos/antvis/F2/actions/artifacts/1685653157/zip',
    headers: {
      accept: 'application/vnd.github.v3+json',
      'user-agent': 'octokit-core.js/4.2.4 Node.js/18.20.8 (linux; x64)',
      authorization: 'token [REDACTED]'
    },
    request: { hook: [Function: bound bound register] }
  }
}
danger-results://tmp/danger-results-f176ded8.json

Generated by 🚫 dangerJS against 6d6b502

@tangying1027
Copy link
Contributor Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 布局组件,支持多种布局模式,这是一个很棒的功能。代码结构清晰,并且包含了相应的测试用例。

我发现了一些潜在的问题,主要集中在布局计算逻辑上,可能会导致在某些场景下布局不正确。其中最主要的问题是手动计算的定位坐标没有被应用到子元素上。此外,栅格布局的计算方式在子元素高度不一时会失效。测试代码中存在过长的延时,可能会影响测试效率。具体细节请看我的评论。修复这些问题后,这个组件会更加健壮和可靠。

Comment on lines +107 to +112
if (tag === Shape) {
if (type === 'circular') {
return <group style={{ x: left, y: top, ...itemStyle }}>{child}</group>;
}
return child;
}

Choose a reason for hiding this comment

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

critical

对于 Shape 类型的子元素,只有在 circular 布局下才应用了定位。在 horizontal, vertical, grid 布局下,计算出的 lefttop 值被忽略了,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>

Choose a reason for hiding this comment

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

critical

lefttop 定位属性没有应用到子组件上。这里计算出的 lefttop 值没有在包裹的 <group> 组件的 style 中使用,导致子组件的位置不正确。

另外,在 getLayoutStyle 方法中使用了 display: 'flex',这与手动计算 left/top 的绝对定位方式存在冲突。建议统一采用一种布局方式。如果采用手动定位,应从 getLayoutStyle 中移除 flex 相关属性,并在这里正确应用 xy 坐标。

        <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);

Choose a reason for hiding this comment

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

high

grid 布局的行定位计算方式存在问题。这里的 top 值是基于 height 计算的,而 height 来自子元素自身的样式,如果子元素没有设置高度,则会取容器的高度。这在栅格布局中会导致问题:

  1. 如果子元素没有高度,height 会是 containerHeight,这通常是不对的。
  2. 如果子元素高度不同,下一行的起始位置会基于前一个元素的高度计算,导致同行元素无法对齐。

建议考虑为 grid 布局增加一个 itemHeight 属性,或者动态计算每行的最大高度来确定下一行的偏移量。

Comment on lines +102 to +103
left = containerWidth / 2 + radius * Math.cos(angle);
top = containerHeight / 2 + radius * Math.sin(angle);

Choose a reason for hiding this comment

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

medium

circular 布局中,子元素是根据其左上角进行定位的。这意味着子元素会以其左上角顶点分布在圆周上。为了视觉上更居中,可以考虑在计算 lefttop 时减去子元素自身宽高的一半。例如 left = ... - childWidth / 2。当然,获取子元素的宽高可能需要一些额外的工作。

const canvas = new Canvas(props);
await canvas.render();

await delay(1000);

Choose a reason for hiding this comment

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

medium

在测试中使用 delay(1000) 等待 1 秒可能没有必要,并且会显著拖慢整个测试套件的执行速度。delay 函数内部已经使用了 setTimeoutrequestAnimationFrame 来确保渲染完成。可以尝试一个更短的延迟,例如 delay(100) 或者更小的值,只要能保证快照正确即可。

Suggested change
await delay(1000);
await delay(100);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant