refactor: improve the management of CellStateStyle.curved
#731
Conversation
Simplify the computation of default values. Add a new story (inspired by the Merge story) to demonstrate the difference of default, curved and rounded edges.
WalkthroughThe pull request primarily improves documentation clarity and consistency across several core modules while also making minor logic adjustments. In the core package, property comments and annotations in the Changes
Suggested labels
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (1)
1-15: Check copyright year.The copyright notice states "2025-present" which appears to be a future date. Consider updating to the current year.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2024-present The maxGraph project Contributors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/view/cell/Cell.ts(2 hunks)packages/core/src/view/geometry/edge/ConnectorShape.ts(1 hunks)packages/core/src/view/geometry/edge/PolylineShape.ts(1 hunks)packages/core/src/view/mixins/EdgeMixin.type.ts(1 hunks)packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)packages/html/stories/Merge.stories.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/html/stories/Merge.stories.ts (1)
packages/html/stories/shared/configure.js (5)
createMainDiv(53-63)container(28-28)createGraphContainer(27-37)createGraphContainer(27-37)style(29-29)
🔇 Additional comments (29)
packages/core/src/view/mixins/EdgeMixin.type.ts (1)
175-176: Good documentation improvement!Adding this important note about the legacy status of this method helps guide developers towards using the newer object parameter approach, which aligns with modern TypeScript practices.
packages/core/src/view/geometry/edge/PolylineShape.ts (1)
77-77: Nice code simplification with optional chaining.The use of optional chaining (
?.) makes the conditional check more concise while maintaining the same behavior, improving code readability.packages/core/src/view/cell/Cell.ts (15)
93-94: Documentation improvement for clarity.The comment now more accurately describes how this property is used by GraphView rather than the deprecated mxGraphView.
102-104: Enhanced property documentation.Adding the
@default nullannotation clearly communicates the default value for this property, improving developer understanding.
108-110: Improved property documentation.The description is now more comprehensive and the default value is explicitly documented.
114-116: Better property documentation.Clearer description with formalized default value documentation.
120-122: More precise property documentation.The updated comment more clearly indicates the purpose of the style property and its default value.
126-128: Clearer vertex property documentation.The comment now explicitly states this property specifies whether the cell is a vertex and includes its default value.
132-134: Improved edge property description.Similar to the vertex property, this update provides a clearer description of the edge property's purpose.
138-140: Enhanced connectable property documentation.The update makes the property description more precise and documents the default value.
144-146: Better visible property documentation.The description is clearer and includes the default value annotation.
150-152: Improved collapsed property documentation.Clearer description with well-formatted default value.
156-158: Enhanced parent property documentation.Added standardized default value documentation.
162-164: Better source property documentation.Default value is now clearly documented.
168-170: Improved target property documentation.Consistent with other property documentation improvements, adding the default value.
174-176: Updated children property documentation.Default empty array is now explicitly documented.
180-182: Enhanced edges property documentation.Clear documentation of the default empty array value.
packages/core/src/view/geometry/edge/ConnectorShape.ts (2)
43-44: Improved JSDoc links in documentation.Using the
{@link ...}syntax for class members makes the documentation more navigable and consistent with best practices.
47-47: Better handling of curved style property with nullish coalescing.The change from implicit boolean conversion to explicit nullish coalescing (
??) makes the code's intent clearer. This ensures thatuseSvgBoundingBoxwill befalsewhenthis.style?.curvedis eitherundefinedornull.packages/html/stories/Merge.stories.ts (5)
19-19: Good addition of constants import.Using the constants module from
@maxgraph/coreis a good practice that improves code maintainability.
32-32: Nice improvement to the import structure.Adding
createMainDivalongsidecreateGraphContainerenhances the demo structure by providing explanatory text for the example.
49-54: Great improvement to user experience.The addition of explanatory text using
createMainDivimproves the Storybook example by clearly describing what the demo illustrates.
74-74: Good use of constants for font styles.Replacing numeric literal
1withconstants.FONT.BOLDimproves code readability and maintainability.Also applies to: 81-81
157-157: Updated return value to include descriptive content.Changing the return value from
containertodivensures the explanatory text is included in the rendered output.packages/html/stories/EdgeCurvedAndRounded.stories.ts (5)
41-60: Well-structured Storybook configuration.The configuration with selectable edge styles (curved, rounded, default) effectively supports the PR objective of demonstrating different edge types.
62-72: Good user experience with descriptive text.The implementation of
createMainDivwith explanatory HTML improves the Storybook example by describing the purpose and usage of the demo.
98-99: Clear implementation of edge style variations.The conditional application of
style.curvedandstyle.roundedbased on the selected style effectively demonstrates the visual differences between edge types.
106-161: Good use of modern vertex insertion API.The story uses the modern object-based parameter approach for
insertVertex, which is more readable and maintainable than the legacy approach.
163-248: Well-structured edge creation with control points.The edges are created using the modern API with explicit geometry points to demonstrate the visual differences between edge styles. This provides a clear visual comparison between curved, rounded, and default edges.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (2)
67-87: Consider using a more specific type for the Template functionThe
Record<string, string>type is very generic for the Template function parameters. Consider creating a more specific interface that describes the expected properties.-const Template = ({ label, ...args }: Record<string, string>) => { +interface TemplateProps { + label?: string; + style: string; + arcSize: string; + rubberBand?: boolean; + [key: string]: any; +} + +const Template = ({ label, ...args }: TemplateProps) => {
88-106: Remove duplicate fontColor propertyThe edge style has
fontColordefined twice, with the first one (line 99) being overridden by the second one (line 101).style = graph.stylesheet.getDefaultEdgeStyle(); style.fontColor = 'gray'; style.fontStyle = constants.FONT.BOLD; - style.fontColor = 'black'; + style.fontColor = 'black'; // This overrides the previous gray setting style.strokeWidth = 2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (6)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (6)
1-15: Copyright notice looks goodThe copyright notice includes the proper Apache License 2.0 details and is dated appropriately.
17-39: Imports are well-organizedThe imports are properly separated by source (core library, shared arguments, shared configuration) and the CSS import is appropriately commented to indicate its purpose.
41-65: Well-structured Storybook configurationThe Storybook configuration is properly structured with a clear title and appropriate argument types. The style and arcSize selectors align with the PR objectives of demonstrating differences between edge styles.
107-168: Good use of batchUpdate for performance optimizationThe use of
graph.batchUpdate()for inserting multiple vertices is a good practice for performance optimization. The vertex creation is well-structured with clear positioning and styling.
169-268: Edge configurations demonstrate the PR objective wellThe edge insertions with various geometry points effectively demonstrate the differences between edge styles (curved, rounded, default) that the PR aims to showcase. The use of control points with
geometry!.pointsis appropriate for visualizing the different edge behaviors.
273-273: Export pattern is consistent with Storybook standardsThe export of the bound Template as 'Default' follows Storybook conventions correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (4)
1-2: Check the copyright yearThe copyright year is set to "2025-present", which is in the future. Consider updating it to the current year or use a more appropriate date range.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2023-present The maxGraph project Contributors
67-74: Add explicit type annotation for the Template function parametersThe function uses a generic
Record<string, string>type, but the parameters could be more specifically typed to match the argTypes defined in the default export. This would improve code readability and type safety.-const Template = ({ label, ...args }: Record<string, string>) => { +interface TemplateArgs { + label?: string; + style: 'curved' | 'rounded' | 'default'; + arcSize: 'default' | '10' | '30' | '50'; + rubberBand?: boolean; + [key: string]: any; +} + +const Template = ({ label, ...args }: TemplateArgs) => {
102-104: Consider using mutually exclusive edge style settingsThe current implementation allows for both
curvedandroundedstyles to be applied simultaneously if the args were manually modified. This might lead to unexpected rendering behavior. Consider using a more explicit approach that ensures only one style is applied.-if (args.style === 'curved') style.curved = true; -if (args.style === 'rounded') style.rounded = true; +// Ensure only one edge style is applied +if (args.style === 'curved') { + style.curved = true; + style.rounded = false; +} else if (args.style === 'rounded') { + style.curved = false; + style.rounded = true; +} else { + // Default style + style.curved = false; + style.rounded = false; +}
89-104: Add comments explaining the purpose of different style configurationsThe styling configuration contains multiple properties, but lacks explanatory comments about their visual effects. Consider adding comments to help users understand the impact of each style property.
// Makes all cells round with a white, bold label let style = graph.stylesheet.getDefaultVertexStyle(); style.shape = 'ellipse'; style.perimeter = Perimeter.EllipsePerimeter; style.fontColor = 'white'; style.gradientColor = 'white'; style.fontStyle = constants.FONT.BOLD; style.fontSize = 14; -// Makes all edge labels gray with a white background +// Configure default edge styles and apply selected edge style (curved/rounded) style = graph.stylesheet.getDefaultEdgeStyle(); style.fontStyle = constants.FONT.BOLD; style.fontColor = 'black'; style.strokeWidth = 2; +// Apply curved style for smooth, curved edges if (args.style === 'curved') style.curved = true; +// Apply rounded style for edges with rounded corners if (args.style === 'rounded') style.rounded = true; +// Apply custom arc size to control the curvature/roundness of edges if (args.arcSize !== 'default') style.arcSize = Number(args.arcSize);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)packages/html/stories/Merge.stories.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/html/stories/Merge.stories.ts
🔇 Additional comments (2)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (2)
41-65: The story configuration looks goodThe Storybook configuration is well-structured with appropriate controls for demonstrating the different edge styles and arc sizes. The use of shared argument types from the imported modules helps maintain consistency with other stories.
110-267: Graph creation and configuration is well-implementedThe code creates a comprehensive example graph with multiple vertices and edges, each with custom styling and geometry points. The use of
graph.batchUpdate()is appropriate for better performance when creating multiple graph elements. The vertex and edge configurations effectively demonstrate the different styling options.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (3)
1-2: Update copyright yearThe copyright header states "2025-present" which is incorrect as we're currently in 2024.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2024-present The maxGraph project Contributors
67-67: Unused parameter in function signatureThe Template function destructures a
labelparameter that isn't used anywhere in the function body.-const Template = ({ label, ...args }: Record<string, string>) => { +const Template = ({ ...args }: Record<string, string>) => {
110-267: Consider refactoring vertex and edge creation for better readabilityThe vertex and edge creation code is repetitive and quite verbose. Consider refactoring to reduce duplication and improve maintainability.
You could create helper functions for inserting vertices and edges with common properties:
function createVertex(graph, id, label, x, y, color) { return graph.insertVertex({ id, value: label, position: [x, y], size: [width, height], style: { fillColor: color, }, }); } function createEdge(graph, id, label, source, target, color, points, alignment = {}) { const edge = graph.insertEdge({ id, value: label, source, target, style: { strokeColor: color, ...alignment, }, }); if (points) { edge.geometry!.points = [new Point(...points)]; } return edge; }Then use them to simplify the batch update section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)packages/html/stories/Merge.stories.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/html/stories/Merge.stories.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (1)
packages/html/stories/Merge.stories.ts (1)
Default(159-159)
🔇 Additional comments (4)
packages/html/stories/EdgeCurvedAndRounded.stories.ts (4)
41-65: LGTM! Well-structured Storybook configurationThe Storybook configuration is well-structured with appropriate controls for demonstrating edge styles and arc sizes. The default values are sensibly set, and the organization aligns with other stories in the codebase.
70-73: LGTM! Clear explanatory textThe descriptive text clearly explains the purpose of the story and how to use the controls to change edge styles.
102-104: LGTM! Clear implementation of edge style configurationThis implementation elegantly applies the selected edge style and arc size based on the Storybook controls, directly addressing the PR objective to demonstrate different edge types.
272-272: LGTM! Standard Storybook patternThe export follows the standard pattern for Storybook stories, consistent with other files in the codebase.
1 participant
Simplify the computation of default values.
Add a new story (inspired by the Merge story) to demonstrate the difference of default, curved and rounded edges.
Screenshots of the new story
PR_731_story_edges_curved_and_rounded.mp4
Tasks
arcSizeSummary by CodeRabbit
Summary by CodeRabbit