feat: increase the fillet configuration of funnel charts #7080
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rubbishmaker, 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!
This pull request introduces a new borderRadius configuration option for funnel charts, allowing users to apply rounded corners to the chart segments. This enhancement is made possible by the addition of a new set of utility functions that can generate SVG paths with rounded corners for any given quadrilateral, providing a more visually appealing and customizable charting experience.
Highlights
- Funnel Chart Customization: Introduces a borderRadius option to the FunnelOptions interface, enabling the application of rounded corners to funnel chart segments.
- Generic Rounded Path Utility: Implements createEdgeBasedRoundedPath in src/shape/utils.ts, a versatile function capable of generating SVG path data for quadrilaterals with specified corner radii.
- Intelligent Vertex Identification: Includes identifyVertices to correctly determine the top-left, top-right, bottom-right, and bottom-left corners of a quadrilateral, ensuring consistent rounding regardless of input point order.
- Edge-Based Corner Calculation: The rounding logic precisely calculates corner points and control points for quadratic Bezier curves, allowing for individual or uniform rounding of corners.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for rounded corners (borderRadius) to funnel charts. This is achieved by introducing new utility functions for creating rounded paths. The implementation is comprehensive, but there are a few areas for improvement. My review includes a high-severity issue regarding vertex identification logic that could lead to malformed shapes, and several medium-severity suggestions to improve code quality, type safety, and maintainability by removing type casts and translating comments to English for consistency.
| const sortedPoints = { | ||
| topLeft: | ||
| identifiedPoints.find((p) => p.position === 'topLeft')?.point || | ||
| points[0], | ||
| topRight: | ||
| identifiedPoints.find((p) => p.position === 'topRight')?.point || | ||
| points[1], | ||
| bottomRight: | ||
| identifiedPoints.find((p) => p.position === 'bottomRight')?.point || | ||
| points[2], | ||
| bottomLeft: | ||
| identifiedPoints.find((p) => p.position === 'bottomLeft')?.point || | ||
| points[3], | ||
| }; |
There was a problem hiding this comment.
The fallback logic for sortedPoints is unsafe and can lead to bugs. It might assign the same point to multiple corners, which would result in a malformed or degenerate shape. For example, if no point is identified as topLeft, the logic falls back to using points[0]. However, points[0] might have already been identified and assigned to another corner (e.g., topRight), causing two corners of the shape to share the same coordinate.
| /** | ||
| * 表示一条边 | ||
| */ | ||
| interface Edge { | ||
| start: Vector2; | ||
| end: Vector2; | ||
| direction: Vector2; // 单位方向向量 | ||
| length: number; | ||
| } | ||
|
|
||
| /** | ||
| * 根据坐标自动识别四边形的顶点位置,无需考虑坐标的顺序 | ||
| */ | ||
| export function identifyVertices(points: Vector2[]) { | ||
| const xs = points.map((p) => p[0]); | ||
| const ys = points.map((p) => p[1]); | ||
| const minX = Math.min(...xs); | ||
| const maxX = Math.max(...xs); | ||
| const minY = Math.min(...ys); | ||
| const maxY = Math.max(...ys); | ||
|
|
||
| const identifiedPoints = points.map((point, index) => { | ||
| const [x, y] = point; | ||
| const distToTopLeft = Math.sqrt((x - minX) ** 2 + (y - minY) ** 2); | ||
| const distToTopRight = Math.sqrt((x - maxX) ** 2 + (y - minY) ** 2); | ||
| const distToBottomRight = Math.sqrt((x - maxX) ** 2 + (y - maxY) ** 2); | ||
| const distToBottomLeft = Math.sqrt((x - minX) ** 2 + (y - maxY) ** 2); | ||
|
|
||
| const distances = { | ||
| topLeft: distToTopLeft, | ||
| topRight: distToTopRight, | ||
| bottomRight: distToBottomRight, | ||
| bottomLeft: distToBottomLeft, | ||
| }; | ||
|
|
||
| const closestCorner = Object.keys(distances).reduce((a, b) => | ||
| distances[a as keyof typeof distances] < | ||
| distances[b as keyof typeof distances] | ||
| ? a | ||
| : b, | ||
| ) as keyof typeof distances; | ||
|
|
||
| return { | ||
| point, | ||
| originalIndex: index, | ||
| position: closestCorner, | ||
| }; | ||
| }); | ||
|
|
||
| const sortedPoints = { | ||
| topLeft: | ||
| identifiedPoints.find((p) => p.position === 'topLeft')?.point || | ||
| points[0], | ||
| topRight: | ||
| identifiedPoints.find((p) => p.position === 'topRight')?.point || | ||
| points[1], | ||
| bottomRight: | ||
| identifiedPoints.find((p) => p.position === 'bottomRight')?.point || | ||
| points[2], | ||
| bottomLeft: | ||
| identifiedPoints.find((p) => p.position === 'bottomLeft')?.point || | ||
| points[3], | ||
| }; | ||
|
|
||
| return { | ||
| topLeft: sortedPoints.topLeft, | ||
| topRight: sortedPoints.topRight, | ||
| bottomRight: sortedPoints.bottomRight, | ||
| bottomLeft: sortedPoints.bottomLeft, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * 创建边对象 | ||
| */ | ||
| function createEdge(start: Vector2, end: Vector2): Edge { | ||
| const dx = end[0] - start[0]; | ||
| const dy = end[1] - start[1]; | ||
| const length = Math.sqrt(dx * dx + dy * dy); | ||
|
|
||
| return { | ||
| start, | ||
| end, | ||
| direction: length > 0 ? [dx / length, dy / length] : [0, 0], | ||
| length, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * 计算边上的圆角信息 | ||
| */ | ||
| function calculateEdgeCorner( | ||
| edge: Edge, | ||
| radius: number, | ||
| isStartCorner: boolean, // true表示在边的起点，false表示在边的终点 | ||
| ): { | ||
| cornerPoint: Vector2; // 圆角在边上的位置 | ||
| hasRadius: boolean; | ||
| actualRadius: number; | ||
| } { | ||
| if (radius <= 0) { | ||
| return { | ||
| cornerPoint: isStartCorner ? edge.start : edge.end, | ||
| hasRadius: false, | ||
| actualRadius: 0, | ||
| }; | ||
| } | ||
|
|
||
| // 限制圆角半径不超过边长的一半 | ||
| const maxRadius = edge.length / 2; | ||
| const actualRadius = Math.min(radius, maxRadius); | ||
|
|
||
| if (actualRadius <= 0) { | ||
| return { | ||
| cornerPoint: isStartCorner ? edge.start : edge.end, | ||
| hasRadius: false, | ||
| actualRadius: 0, | ||
| }; | ||
| } | ||
|
|
||
| // 计算圆角在边上的位置 | ||
| let cornerPoint: Vector2; | ||
| if (isStartCorner) { | ||
| // 从起点沿边方向移动radius距离 | ||
| cornerPoint = [ | ||
| edge.start[0] + edge.direction[0] * actualRadius, | ||
| edge.start[1] + edge.direction[1] * actualRadius, | ||
| ]; | ||
| } else { | ||
| // 从终点沿边反方向移动radius距离 | ||
| cornerPoint = [ | ||
| edge.end[0] - edge.direction[0] * actualRadius, | ||
| edge.end[1] - edge.direction[1] * actualRadius, | ||
| ]; | ||
| } | ||
|
|
||
| return { | ||
| cornerPoint, | ||
| hasRadius: true, | ||
| actualRadius, | ||
| }; | ||
| } | ||
| /** | ||
| * 生成基于边的圆角路径 | ||
| */ | ||
| export function createEdgeBasedRoundedPath( | ||
| points: Vector2[], | ||
| borderRadius: | ||
| | number | ||
| | { | ||
| topLeft?: number; | ||
| topRight?: number; | ||
| bottomLeft?: number; | ||
| bottomRight?: number; | ||
| }, | ||
| ): string { | ||
| // 1. 识别顶点位置 | ||
| const vertices = identifyVertices(points); | ||
|
|
||
| // 2. 获取圆角配置 | ||
| const getRadius = (corner: string) => { | ||
| if (typeof borderRadius === 'number') { | ||
| return borderRadius; | ||
| } | ||
| return (borderRadius as any)?.[corner] || 0; | ||
| }; | ||
|
|
||
| const radii = { | ||
| topLeft: getRadius('topLeft'), | ||
| topRight: getRadius('topRight'), | ||
| bottomRight: getRadius('bottomRight'), | ||
| bottomLeft: getRadius('bottomLeft'), | ||
| }; | ||
|
|
||
| // 3. 如果所有圆角都为0，返回简单路径 | ||
| if (Object.values(radii).every((r) => r === 0)) { | ||
| const { topLeft, topRight, bottomRight, bottomLeft } = vertices; | ||
| return `M ${topLeft[0]} ${topLeft[1]} L ${topRight[0]} ${topRight[1]} L ${bottomRight[0]} ${bottomRight[1]} L ${bottomLeft[0]} ${bottomLeft[1]} Z`; | ||
| } | ||
|
|
||
| // 4. 创建四条边 | ||
| const edges = [ | ||
| createEdge(vertices.topLeft, vertices.topRight), // 上边 | ||
| createEdge(vertices.topRight, vertices.bottomRight), // 右边 | ||
| createEdge(vertices.bottomRight, vertices.bottomLeft), // 下边 | ||
| createEdge(vertices.bottomLeft, vertices.topLeft), // 左边 | ||
| ]; | ||
|
|
||
| const edgeNames = ['top', 'right', 'bottom', 'left']; | ||
| const cornerNames = ['topLeft', 'topRight', 'bottomRight', 'bottomLeft']; | ||
|
|
||
| // 5. 计算每条边上的圆角点 | ||
| const edgeCorners = edges.map((edge, edgeIndex) => { | ||
| const startCornerName = cornerNames[edgeIndex]; // 边起点对应的角 | ||
| const endCornerName = cornerNames[(edgeIndex + 1) % 4]; // 边终点对应的角 | ||
|
|
||
| const startRadius = radii[startCornerName as keyof typeof radii]; | ||
| const endRadius = radii[endCornerName as keyof typeof radii]; | ||
|
|
||
| return { | ||
| edge, | ||
| edgeName: edgeNames[edgeIndex], | ||
| startCorner: calculateEdgeCorner(edge, startRadius, true), // 边起点的圆角 | ||
| endCorner: calculateEdgeCorner(edge, endRadius, false), // 边终点的圆角 | ||
| startCornerName, | ||
| endCornerName, | ||
| }; | ||
| }); | ||
|
|
||
| // 6. 生成SVG路径 | ||
| const pathCommands: string[] = []; | ||
|
|
||
| // 从第一条边的起点圆角开始 | ||
| const firstEdge = edgeCorners[0]; | ||
| pathCommands.push( | ||
| `M ${firstEdge.startCorner.cornerPoint[0]} ${firstEdge.startCorner.cornerPoint[1]}`, | ||
| ); | ||
|
|
||
| for (let i = 0; i < 4; i++) { | ||
| const currentEdge = edgeCorners[i]; | ||
| const nextEdge = edgeCorners[(i + 1) % 4]; | ||
|
|
||
| // 沿着当前边绘制到终点圆角 | ||
| pathCommands.push( | ||
| `L ${currentEdge.endCorner.cornerPoint[0]} ${currentEdge.endCorner.cornerPoint[1]}`, | ||
| ); | ||
|
|
||
| // 在角点处绘制圆角弧线 | ||
| const cornerVertex = edges[i].end; // 当前边的终点就是角点 | ||
| const hasCornerRadius = currentEdge.endCorner.hasRadius; | ||
|
|
||
| if (hasCornerRadius) { | ||
| // 绘制圆角弧线：从当前边的终点圆角到下一条边的起点圆角 | ||
| const controlPoint = cornerVertex; // 使用角点作为控制点 | ||
| pathCommands.push( | ||
| `Q ${controlPoint[0]} ${controlPoint[1]} ${nextEdge.startCorner.cornerPoint[0]} ${nextEdge.startCorner.cornerPoint[1]}`, | ||
| ); | ||
| } else { | ||
| // 没有圆角，直接连到下一条边的起点 | ||
| pathCommands.push( | ||
| `L ${nextEdge.startCorner.cornerPoint[0]} ${nextEdge.startCorner.cornerPoint[1]}`, | ||
| ); | ||
| } | ||
| } | ||
| pathCommands.push('Z'); | ||
| const finalPath = pathCommands.join(' '); | ||
| return finalPath; | ||
| } |
There was a problem hiding this comment.
The new functions and interfaces in this file have comments written in Chinese. For consistency with the rest of the codebase, which is in English, and to improve maintainability for all contributors, please translate these comments to English.
Examples:
- Line 243:
* 表示一条边 - Line 253:
* 根据坐标自动识别四边形的顶点位置,无需考虑坐标的顺序 - Line 336:
isStartCorner: boolean, // true表示在边的起点，false表示在边的终点
| const getRadius = (corner: string) => { | ||
| if (typeof borderRadius === 'number') { | ||
| return borderRadius; | ||
| } | ||
| return (borderRadius as any)?.[corner] || 0; | ||
| }; |
There was a problem hiding this comment.
The use of as any bypasses TypeScript's type safety and should be avoided. It can hide potential bugs and makes the code harder to maintain.
| const getRadius = (corner: string) => { | |
| if (typeof borderRadius === 'number') { | |
| return borderRadius; | |
| } | |
| return (borderRadius as any)?.[corner] || 0; | |
| }; | |
| const getRadius = (corner: 'topLeft' | 'topRight' | 'bottomRight' | 'bottomLeft') => { | |
| if (typeof borderRadius === 'number') { | |
| return borderRadius; | |
| } | |
| return borderRadius?.[corner] || 0; | |
| }; |
src/shape/utils.ts
Outdated
| ]; | ||
|
|
||
| const edgeNames = ['top', 'right', 'bottom', 'left']; | ||
| const cornerNames = ['topLeft', 'topRight', 'bottomRight', 'bottomLeft']; |
There was a problem hiding this comment.
The type casts as keyof typeof radii can be avoided by providing a more specific type for cornerNames. This improves type safety and makes the code easier to read and maintain.
| const cornerNames = ['topLeft', 'topRight', 'bottomRight', 'bottomLeft']; | |
| const cornerNames = ['topLeft', 'topRight', 'bottomRight', 'bottomLeft'] as const; |
|
加一下测试用例，包括 coordinate 非 transpose 的情况，避免后续修改导致的触碰。 |
请教下G2单测怎么写，工作日太忙了，打算周末来搞定 |
参考这个 pr 中的单测写法 #7004 |
3 participants
Checklist
npm testpassesDescription of change