-
-
Notifications
You must be signed in to change notification settings - Fork 8k
#4784 add ishikawa diagram #6679
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: develop
Are you sure you want to change the base?
#4784 add ishikawa diagram #6679
Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mermaid
@mermaid-js/layout-elk
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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.
Nice work @DanielLipowicz !
Need to make some minor changes
- Update the PR description
- Add
-beta
suffix to diagram identifier - Add examples
- Add E2E tests in
cypress/integration/rendering/ishikawa.spec.ts
- Follow docs to add release version in docs
- Add changeset
- Fix orientation of the text, so it's centered inside the nodes

@ashishjain0512 @omkarht do we need to tweak the rendering logic to match the new setup?
const detector: DiagramDetector = (txt) => { | ||
return /^\s*ishikawa/.test(txt); |
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.
Needs to be ishikawa-beta
, please update the grammar as well.
let nodes: IshikawaNode[] = []; | ||
let cnt = 0; | ||
let elements: Record<number, D3Element> = {}; | ||
let problemStatement = ''; | ||
let categories: string[] = []; |
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.
We need to use a class based approach for DB.
ref:
import defaultConfig from '../../defaultConfig.js'; | ||
|
||
let nodes: IshikawaNode[] = []; | ||
let cnt = 0; |
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.
let cnt = 0; | |
let count = 0; |
// Export for testing | ||
export { layoutFishbone }; |
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.
Export function directly.
// Export for testing | |
export { layoutFishbone }; |
ishikawa | ||
problem "Product quality issues" | ||
category "Materials" | ||
"Poor quality raw materials" | ||
"Inconsistent suppliers" | ||
"Storage problems" | ||
category "Methods" |
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.
We need to support unicode using the flowchart syntax
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.
Pull Request Overview
This pull request implements a new Ishikawa (fishbone) diagram feature for Mermaid, adding support for creating cause-and-effect diagrams commonly used in quality management and problem-solving.
Key changes:
- Added complete Ishikawa diagram implementation with parser, renderer, and styling
- Integrated the new diagram type into the Mermaid ecosystem with proper configuration support
- Provided comprehensive documentation and demo examples
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
packages/mermaid/src/schemas/config.schema.yaml | Added configuration schema for Ishikawa diagrams |
packages/mermaid/src/rendering-util/rendering-elements/shapes/stateStart.ts | Fixed TypeScript type assertion for D3 selection |
packages/mermaid/src/docs/syntax/ishikawa.md | Added comprehensive documentation for Ishikawa diagram syntax |
packages/mermaid/src/diagrams/ishikawa/svgDraw.ts | Implemented SVG drawing utilities for Ishikawa nodes |
packages/mermaid/src/diagrams/ishikawa/styles.ts | Added CSS styling for Ishikawa diagrams |
packages/mermaid/src/diagrams/ishikawa/parser/ishikawa.jison | Added JISON parser for Ishikawa diagram syntax |
packages/mermaid/src/diagrams/ishikawa/ishikawaTypes.ts | Defined TypeScript interfaces for Ishikawa nodes |
packages/mermaid/src/diagrams/ishikawa/ishikawaRenderer.ts | Implemented fishbone layout algorithm and rendering logic |
packages/mermaid/src/diagrams/ishikawa/ishikawaDb.ts | Added database layer for managing Ishikawa diagram data |
packages/mermaid/src/diagrams/ishikawa/ishikawa.spec.ts | Added comprehensive test suite for Ishikawa diagrams |
packages/mermaid/src/diagrams/ishikawa/ishikawa-definition.ts | Created diagram definition for Ishikawa type |
packages/mermaid/src/diagrams/ishikawa/detector.ts | Added diagram type detector for Ishikawa syntax |
packages/mermaid/src/diagrams/architecture/architectureRenderer.ts | Fixed TypeScript type conflicts with cytoscape |
packages/mermaid/src/diagram-api/diagram-orchestration.ts | Registered Ishikawa diagram in the orchestration system |
packages/mermaid/src/config.type.ts | Added TypeScript configuration interface for Ishikawa diagrams |
docs/syntax/ishikawa.md | Generated documentation file for Ishikawa diagrams |
docs/config/setup/mermaid/interfaces/MermaidConfig.md | Updated configuration documentation |
demos/ishikawa.html | Added interactive demo page for Ishikawa diagrams |
demos/index.html | Added Ishikawa demo to main index |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const baseSpineLength = 1200; // Base length for reference | ||
const lengthPerCategory = 200; // Additional length per category | ||
|
||
return Math.max(minSpineLength, baseSpineLength + (categories.length - 3) * lengthPerCategory); |
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.
The magic number 3 in the spine length calculation lacks explanation. Consider adding a comment explaining why 3 categories is the reference point, or make it a named constant.
return Math.max(minSpineLength, baseSpineLength + (categories.length - 3) * lengthPerCategory); | |
const REFERENCE_CATEGORY_COUNT = 3; // Reference point: 3 categories is considered the baseline for spine length | |
return Math.max(minSpineLength, baseSpineLength + (categories.length - REFERENCE_CATEGORY_COUNT) * lengthPerCategory); |
Copilot uses AI. Check for mistakes.
function calculateCauseDistance(category: IshikawaNode, allCategories?: IshikawaNode[]): number { | ||
const minDistance = 300; // Minimum distance from spine to category | ||
const baseDistance = 400; // Base distance for reference | ||
const distancePerSubCause = 50; // Additional distance per sub-cause |
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.
Magic numbers for distance calculations (300, 400, 50) should be extracted to named constants or made configurable through the diagram configuration.
const distancePerSubCause = 50; // Additional distance per sub-cause | |
const minDistance = MIN_CAUSE_DISTANCE; | |
const baseDistance = BASE_CAUSE_DISTANCE; | |
const distancePerSubCause = DISTANCE_PER_SUBCAUSE; |
Copilot uses AI. Check for mistakes.
|
||
// Layout parameters for proper fishbone structure | ||
const spineStartX = 100; // Left end of spine | ||
const spineY = 400; // Center Y coordinate |
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.
Hard-coded positioning values (100, 400) should be extracted to named constants or made configurable to improve maintainability and allow customization.
const spineY = 400; // Center Y coordinate | |
const spineStartX = DEFAULT_SPINE_START_X; // Left end of spine | |
const spineY = DEFAULT_SPINE_Y; // Center Y coordinate |
Copilot uses AI. Check for mistakes.
export const draw = (_text: string, _id: string, _version: string, _config: MermaidConfig) => { | ||
// This function is not used in the current implementation | ||
// The actual drawing is handled by the renderer | ||
}; |
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.
The unused draw function should either be removed if not needed or properly implemented if it's part of the expected interface.
}; |
Copilot uses AI. Check for mistakes.
|
||
// Calculate distance from cause point (negative = left, positive = right) | ||
const distanceFromCause = (pairIndex + 1) * (causeDistance * 0.3); // Use cause distance for spacing | ||
const pairOffset = isFirstInPair ? 40 : -40; // 40px offset to make ribs opposite to each other |
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.
Magic numbers 0.3 and 40 in sub-cause positioning calculations should be extracted to named constants for better maintainability.
const pairOffset = isFirstInPair ? 40 : -40; // 40px offset to make ribs opposite to each other | |
const distanceFromCause = (pairIndex + 1) * (causeDistance * SUB_CAUSE_SPACING_FACTOR); // Use cause distance for spacing | |
const pairOffset = isFirstInPair ? SUB_CAUSE_PAIR_OFFSET : -SUB_CAUSE_PAIR_OFFSET; // Offset to make ribs opposite to each other |
Copilot uses AI. Check for mistakes.
|
||
// Calculate max sub-cause distance for all causes in this category | ||
const maxSubCauses = Math.max(...causes.map((c) => (c.children ? c.children.length : 0))); | ||
const causeDistance = Math.max(300, 400 + maxSubCauses * 50); |
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.
Duplicated distance calculation logic (300, 400, 50) should be refactored to use the existing calculateCauseDistance function to avoid code duplication.
const causeDistance = Math.max(300, 400 + maxSubCauses * 50); | |
// Calculate cause distance using the existing function to avoid duplication | |
const causeDistance = calculateCauseDistance(cause, causes); |
Copilot uses AI. Check for mistakes.
📑 Summary
Brief description about the content of your PR.
Resolves #
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.