Skip to content

Conversation

@lyjeileen
Copy link
Collaborator

@lyjeileen lyjeileen commented May 28, 2024

Change

  • Add mermaidViz component
  • Add stories and tests for it

Screenshot

Light mode

Screenshot 2024-05-29 at 4 02 50 PM Screenshot 2024-05-29 at 4 02 58 PM Screenshot 2024-05-29 at 4 03 03 PM

Dark mode

Screenshot 2024-05-29 at 4 03 25 PM

Error

Screenshot 2024-05-31 at 5 32 15 PM

export type VegaLiteData = VegaLiteFormat & Updates<VegaLiteFormat>

export interface MermaidFormat extends DataFormat {
/** Follow mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** Follow mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
/** Follow Mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */

export interface MermaidFormat extends DataFormat {
/** Follow mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
code: string
/** Follow mermaid's [documentation](https://mermaid.js.org/config/setup/README.html) to provide a config object. It's used for additional customizations. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** Follow mermaid's [documentation](https://mermaid.js.org/config/setup/README.html) to provide a config object. It's used for additional customizations. */
/** Follow Mermaid's [documentation](https://mermaid.js.org/config/setup/README.html) to provide a config object. It's used for additional customizations. */

tags: ['autodocs'],
parameters: {
layout: 'centered',
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
},
},
decorators: [
(Story: StoryFn) => {
return (
<div
style={{
width: 'clamp(250px, 70vw, 1000px)',
}}
>
<Story />
</div>
)
},
]

Comment on lines 20 to 32
const decorators = [
(Story: StoryFn) => {
return (
<div
style={{
width: 'clamp(250px, 70vw, 1000px)',
}}
>
<Story />
</div>
)
},
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const decorators = [
(Story: StoryFn) => {
return (
<div
style={{
width: 'clamp(250px, 70vw, 1000px)',
}}
>
<Story />
</div>
)
},
]

},
]

export const classDiagram = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const classDiagram = {
export const ClassDiagram = {

C -->|Two| E[iPhone]
C -->|Three| F[fa:fa-car Car]`,
},
decorators,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
decorators,

PRODUCT-CATEGORY ||--|{ PRODUCT : contains
PRODUCT ||--o{ ORDER-ITEM : "ordered in"`,
},
decorators,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
decorators,

decorators,
}

export const error = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const error = {
export const Error = {

args: {
code: '',
},
decorators,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
decorators,

)

cy.get(mermaidContainer).should('exist')
cy.get(mermaidContainer).contains('Christmas Shopping')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do this to be explicit about assertions?

Suggested change
cy.get(mermaidContainer).contains('Christmas Shopping')
cy.get(mermaidContainer).should('contain', 'Christmas Shopping')

@kaseyvee
Copy link
Collaborator

kaseyvee commented May 29, 2024

I think the neutral theme mermaid.js provides would be a good option for minimally handling the theme.

@lyjeileen
Copy link
Collaborator Author

lyjeileen commented May 29, 2024

I think the neutral theme mermaid.js provides would be a good option for minimally handling the theme.

I was thinking using default for light theme and dark for dark theme. I changed default to neutral for light theme.

@lyjeileen lyjeileen marked this pull request as ready for review May 29, 2024 23:06
...props.config,
})

mermaid.contentLoaded()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be using mermaid.run instead. It allows more flexibility for the selectors and suppressing errors so that we can show custom error messages.

Refer - https://mermaid.js.org/config/usage.html#using-mermaid-run

Comment on lines 162 to 163
/** Follow Mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
code: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Follow Mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
code: string
/** Diagram definition following [Mermaid's syntax](https://mermaid.js.org/intro/syntax-reference.html#syntax-structure). The use of [these](https://mermaid.js.org/intro/syntax-reference.html#diagram-breaking) words or symbols can break diagrams. */
diagram: string

export interface MermaidFormat extends DataFormat {
/** Follow Mermaid's [documentation](https://mermaid.js.org/intro/) to provide a code string. */
code: string
/** Follow Mermaid's [documentation](https://mermaid.js.org/config/setup/README.html) to provide a config object. It's used for additional customizations. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Follow Mermaid's [documentation](https://mermaid.js.org/config/setup/README.html) to provide a config object. It's used for additional customizations. */
/** Configuration for altering and customizing Mermaid Diagrams. Refer [Mermaid docs](https://mermaid.js.org/config/schema-docs/config.html) for details. */


/** The `MermaidViz` component leverages [Mermaid.js](https://mermaid.js.org/) to create dynamic and interactive diagrams, including flowcharts, sequence diagrams, class diagrams, and more. It is ideal for visualizing complex data and processes in a clear and structured manner. */
function MermaidViz(props: MermaidData) {
const chartRef = useRef(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the name for the variable and also specify the type of useRef

Comment on lines 30 to 34
<Stack
direction="column"
className="rustic-mermaid-container"
data-cy="mermaid-container"
>
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason for using Stack instead of a simple div ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to avoid adding display: flex; flex-direction: column by using Stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

In a div, elements are rendered one below the other. why do we need flex ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want mermaid div takes up the remaining space of its parent container. I used flex:1 in mermaid div and in order to make it work, its parent container needs to have display: flex

Comment on lines 6 to 16
.rustic-mermaid-container div:last-of-type {
flex: 1;
min-height: 0;
display: flex;
justify-content: center;
}

.rustic-mermaid-container div:last-of-type svg {
height: 100%;
width: 100%;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use a custom class name. Also, its not clear why we are setting min-height to 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will use a custom className. For min-height: 0, I can't make the diagram's height responsive without it.

)
})
}
}, [mermaidTheme])
Copy link
Contributor

@Shiti Shiti May 31, 2024

Choose a reason for hiding this comment

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

I think we should make mermaidTheme a local variable inside useEffect and set rusticTheme as dependency

Suggested change
}, [mermaidTheme])
}, [rusticTheme])

I have noticed a similar issue with other components as well, some of them do not reload on theme change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using rusticTheme doesn't fix the theme toggle issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah thats also a constant.

if (mermaidRef.current) {
mermaid.initialize({
theme: mermaidTheme,
themeVariables: { fontFamily: 'Inter' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont hard-code font.

.catch(() => {
setIsProcessed(true)
setErrorMessage(
'An error occurred while rendering the diagram. Please check the syntax.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'An error occurred while rendering the diagram. Please check the syntax.'
'Failed to render the diagram.'


mermaid
.run({
querySelector: '.rustic-mermaid',
Copy link
Contributor

Choose a reason for hiding this comment

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

This query selector is too generic and will apply to all the elements on the page with this class.

@Shiti Shiti requested a review from kaseyvee June 2, 2024 03:07
@Shiti Shiti merged commit 816122d into rustic-ai:main Jun 3, 2024
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.

3 participants