feat(template): Cylinders 3d#6
Conversation
yvonneyx
commented
Nov 3, 2025
yvonneyx
commented
Summary of ChangesHello @yvonneyx, 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 significantly enhances the infographic library by introducing a novel 3D cylinder sequence visualization. This new structure provides a visually rich and dynamic way to represent ordered data, allowing for customizable depth, height, and item alignment. The addition of an 'alternating' item component further refines the presentation, offering a clean and organized display for associated data points. This feature expands the toolkit for creating engaging and informative infographics. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Cylinders 3d' template, which includes a new AlternatingItem component and a sequence-cylinders-3d structure. The implementation is comprehensive and visually impressive. My review focuses on improving robustness, maintainability, and correctness. I've identified a critical issue that could cause a crash with empty data, a misconfiguration in the new template, and opportunities to reduce code duplication and improve clarity by removing magic numbers.
| const itemBounds = getElementBounds( | ||
| <Item indexes={[0]} data={data} datum={items[0]} positionH="center" />, | ||
| ); |
There was a problem hiding this comment.
The code assumes the items array is not empty when calculating itemBounds. If data.items is empty or not provided, items[0] will be undefined, which will likely cause a runtime error inside getElementBounds or the Item component. You should add a guard to handle cases where the items array is empty.
| const itemBounds = getElementBounds( | |
| <Item indexes={[0]} data={data} datum={items[0]} positionH="center" />, | |
| ); | |
| const itemBounds = items.length | |
| ? getElementBounds( | |
| <Item indexes={[0]} data={data} datum={items[0]} positionH="center" />, | |
| ) | |
| : { width: 0, height: 0 }; |
| 'sequence-cylinders-3d-simple': { | ||
| design: { | ||
| title: 'default', | ||
| structure: { type: 'sequence-cylinders-3d', gapY: 20 }, |
There was a problem hiding this comment.
The sequence-cylinders-3d structure is configured with a gapY: 20 property. However, the sequenceCylinders3d component does not define or accept a gapY prop. This property will be ignored and suggests a configuration error. Please review the available props for the component and use a valid one, such as depthSpacing, or remove this property.
| if (!showIcon || !icon) { | ||
| return ( | ||
| <Group {...restProps}> | ||
| <Group> | ||
| <ItemLabel | ||
| indexes={indexes} | ||
| width={width} | ||
| alignHorizontal={textAlign} | ||
| alignVertical="center" | ||
| fill={labelColor} | ||
| > | ||
| {label} | ||
| </ItemLabel> | ||
| <ItemDesc | ||
| indexes={indexes} | ||
| width={width} | ||
| y={labelBounds.height + gap} | ||
| alignHorizontal={textAlign} | ||
| alignVertical="top" | ||
| fill={themeColors.colorTextSecondary} | ||
| > | ||
| {desc} | ||
| </ItemDesc> | ||
| </Group> | ||
| </Group> | ||
| ); | ||
| } | ||
|
|
||
| const iconBounds = getElementBounds(iconContent); | ||
| const textWidth = Math.max(width - iconBounds.width - gap, 0); | ||
|
|
||
| return ( | ||
| <Group {...restProps}> | ||
| <FlexLayout flexDirection="row" gap={gap} alignItems="flex-start"> | ||
| {isOdd ? ( | ||
| <> | ||
| <Group> | ||
| <ItemLabel | ||
| indexes={indexes} | ||
| width={textWidth} | ||
| alignHorizontal="right" | ||
| alignVertical="center" | ||
| fill={labelColor} | ||
| > | ||
| {label} | ||
| </ItemLabel> | ||
| <ItemDesc | ||
| indexes={indexes} | ||
| width={textWidth} | ||
| y={labelBounds.height + gap} | ||
| alignHorizontal="right" | ||
| alignVertical="top" | ||
| fill={themeColors.colorTextSecondary} | ||
| > | ||
| {desc} | ||
| </ItemDesc> | ||
| </Group> | ||
| {iconContent} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {iconContent} | ||
| <Group> | ||
| <ItemLabel | ||
| indexes={indexes} | ||
| width={textWidth} | ||
| alignHorizontal="left" | ||
| alignVertical="center" | ||
| fill={labelColor} | ||
| > | ||
| {label} | ||
| </ItemLabel> | ||
| <ItemDesc | ||
| indexes={indexes} | ||
| width={textWidth} | ||
| y={labelBounds.height + gap} | ||
| alignHorizontal="left" | ||
| alignVertical="top" | ||
| fill={themeColors.colorTextSecondary} | ||
| > | ||
| {desc} | ||
| </ItemDesc> | ||
| </Group> | ||
| </> | ||
| )} | ||
| </FlexLayout> | ||
| </Group> | ||
| ); |
There was a problem hiding this comment.
There is significant code duplication in rendering the text content (ItemLabel and ItemDesc). The logic is repeated for cases with and without an icon. This can be refactored by extracting the text block into a shared variable, which will make the component more maintainable and easier to read.
const hasIcon = showIcon && icon;
const iconBounds = hasIcon && iconContent ? getElementBounds(iconContent) : { width: 0 };
const textWidth = hasIcon ? Math.max(width - iconBounds.width - gap, 0) : width;
const textBlock = (
<Group>
<ItemLabel
indexes={indexes}
width={textWidth}
alignHorizontal={textAlign}
alignVertical="center"
fill={labelColor}
>
{label}
</ItemLabel>
<ItemDesc
indexes={indexes}
width={textWidth}
y={labelBounds.height + gap}
alignHorizontal={textAlign}
alignVertical="top"
fill={themeColors.colorTextSecondary}
>
{desc}
</ItemDesc>
</Group>
);
if (!hasIcon) {
return <Group {...restProps}>{textBlock}</Group>;
}
return (
<Group {...restProps}>
<FlexLayout flexDirection="row" gap={gap} alignItems="flex-start">
{isOdd ? (
<>
{textBlock}
{iconContent}
</>
) : (
<>
{iconContent}
{textBlock}
</>
)}
</FlexLayout>
</Group>
);
| cylinderRy?: number; | ||
| baseHeight?: number; | ||
| heightIncrement?: number; | ||
| horizontalSpacing?: number; |
There was a problem hiding this comment.
The prop horizontalSpacing is defined in the sequenceCylinders3dProps interface but it is never used within the component. This can be misleading for developers. Please either implement the functionality for this prop or remove it if it's not needed.
| fontFamily="Arial Black, sans-serif" | ||
| fontSize={32} | ||
| fontWeight={900} |
There was a problem hiding this comment.
The fontFamily, fontSize, and fontWeight for the cylinder number text are hardcoded. This prevents customization through themes and can lead to inconsistencies with the overall design. It would be better to source these values from the theme object.
| const btnNodes: JSXElement[] = [ | ||
| <BtnRemove | ||
| indexes={[index]} | ||
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | ||
| y={itemPos.y + itemBounds.height + 10} | ||
| />, | ||
| <BtnAdd | ||
| indexes={[index]} | ||
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | ||
| y={itemPos.y - btnBounds.height - 10} | ||
| />, | ||
| ]; |
There was a problem hiding this comment.
The value 10 is used as a magic number for the vertical margin when positioning the BtnRemove and BtnAdd components. This makes the code harder to understand and maintain. It's recommended to extract this value into a named constant (e.g., BTN_VERTICAL_MARGIN) to improve readability.
| const btnNodes: JSXElement[] = [ | |
| <BtnRemove | |
| indexes={[index]} | |
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | |
| y={itemPos.y + itemBounds.height + 10} | |
| />, | |
| <BtnAdd | |
| indexes={[index]} | |
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | |
| y={itemPos.y - btnBounds.height - 10} | |
| />, | |
| ]; | |
| const BTN_VERTICAL_MARGIN = 10; | |
| const btnNodes: JSXElement[] = [ | |
| <BtnRemove | |
| indexes={[index]} | |
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | |
| y={itemPos.y + itemBounds.height + BTN_VERTICAL_MARGIN} | |
| />, | |
| <BtnAdd | |
| indexes={[index]} | |
| x={itemPos.x + itemBounds.width / 2 - btnBounds.width / 2} | |
| y={itemPos.y - btnBounds.height - BTN_VERTICAL_MARGIN} | |
| />, | |
| ]; |
| }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
可以看看 item 和 structure 和其他设计资产组合有没有好看的～
yvonneyx
force-pushed
the
cylinders-3d
branch
from
c9300c9 to
e96db37
Compare
2 participants
