Skip to content
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

build: simple setup vite for playground #52

Merged
merged 38 commits into from
May 2, 2024

Conversation

igorwessel
Copy link
Contributor

@igorwessel igorwessel commented Apr 12, 2024

Closes #36

Description

I've modified how we're creating IDs for the diagrams to be more consistent, without needing to define an index offset. This was necessary to easily accommodate Hot Module Replacement (HMR).

I've added basic HMR support in the test files. I didn't feel the need to implement it for when there's a removal/etc... as we're only going to use the playground for testing.

I've kept the same format that Parcel was using so we don't have to make any adjustments on the Vercel side.

Test Cases

Testcases files only change respective name and definition

diagram.mov

Only rerender current active test file

active-diagram.mov

Except for React Components and the Testcase files, all others trigger a full page reload with HMR.

another-files.mov

The production build we can test locally through preview and the pipeline will also allow us to test.

Copy link

vercel bot commented Apr 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
mermaid-to-excalidraw ✅ Ready (Inspect) Visit Preview May 1, 2024 2:41pm

playground/index.ts Outdated Show resolved Hide resolved
playground/index.ts Outdated Show resolved Hide resolved
playground/testcases/class.ts Show resolved Hide resolved
playground/vite.config.ts Show resolved Hide resolved
playground/tsconfig.json Show resolved Hide resolved
playground/index.ts Outdated Show resolved Hide resolved
playground/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

looking great @igorwessel
Left few comments and here are some additional things that needs to be fixed

  1. Updating custom test case should update mermaid output
  2. Updating custom test case should update excalidraw diagram

playground/context/excalidraw.ts Outdated Show resolved Hide resolved
playground/index.tsx Outdated Show resolved Hide resolved
playground/Mermaid.tsx Outdated Show resolved Hide resolved
@igorwessel
Copy link
Contributor Author

igorwessel commented Apr 25, 2024

For the custom testcase, let's render the mermaid output and excalidraw diagram only after the render button is clicked, otherwise it will always show up error when user is typing which is annoying.

Indeed HAHAHA

If I update a testcase file and introduce error, it doesn't show up in the playground, probably coz the error is always set to null

I noticed that we don't have the error inside the Testfile component, so the error was shown generally on the website.

I added the error but it will only display for the currently active test case. do you think it makes sense to have something more granular?

@igorwessel igorwessel requested a review from ad1992 April 25, 2024 11:09
Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

The code is much cleaner now @igorwessel :)
Left a few comments.

Comment on lines 96 to 99
activeTestcase={
activeTestcase.current?.[0] === index
? activeTestcase.current[1]
: undefined
Copy link
Member

Choose a reason for hiding this comment

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

this is confusing, lets have rename to activeTestCaseIndex and only store the active test case index here.
You can instead compare it with index here and pass the prop accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ad1992 now I'm a bit confused haha.
The idea was to store both the index of the test files and the index of the active testcase. Without this, it doesn't know that it's file X that has the active test, and it ends up requesting to render the Excalidraw for all files not only for current active testcase.

Copy link
Member

Choose a reason for hiding this comment

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

@igorwessel to fix that can we have a base index instead? It starts from 0 and keeps on incrementing until all test cases are rendered.

Copy link
Contributor Author

@igorwessel igorwessel Apr 29, 2024

Choose a reason for hiding this comment

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

@ad1992 I will try this approach it should work. if don't maybe we can just destructure and name these variables?

const [activeTestCaseFileIndex, activeTestCaseIndex] = activeTestCase.current

activeTestCaseFileIndex === fileIndex ? activeTestCaseIndex : undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ad1992 I change the mind of some of the responsibility and understood that the duty of triggering the active test should lie with TestCases, not with SingleTestCase
done in 378eaf9

onChange(definition, false);
}}
testcases={testcases}
error={!isCustomTest ? error : null}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error={!isCustomTest ? error : null}
error={isCustomTest ? null : error}

better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in e540933

activeTestcase?: number;
}

const Testcase = ({
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this component to a file SingleTestCase.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in c0752ad

testcases: { name: string; definition: string }[];
onChange: (definition: string, activeTestcaseIndex: number) => void;
error: string | null;
activeTestcase?: number;
Copy link
Member

Choose a reason for hiding this comment

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

rename to activeTestCaseIndex as its the index and not the testcase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in d3716a2


<MermaidDiagram definition={definition} id={id} />

{error && activeTestcase === index && (
Copy link
Member

Choose a reason for hiding this comment

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

This would mean it shows error only for active testcase lets make it render error irrespective of the condition ?

Currently If I edit a testcase file such with errored syntax, the text gets updated however mermaid diagram doesn't rerender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ad1992 I was thinking about this, and the idea of ​​just showing for the active test case was to not have to store the individual errors for each testcase.

Today we don't have the reference so the last error would be overwritten, so that error structure in the App file would become a kind of multidimensional array:

[
  [
    0 // index testcases files, 
    0 // index testcase inside testscases file
  ]
]

Copy link
Member

Choose a reason for hiding this comment

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

Nah let's not complicate it, we might add error field later in test case file itself if needed, that would simplify this.

However for now instead of not rendering anything during error, how about saying "Error in Rendering Mermaid Diagram" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ad1992 I realized that it shouldn't be the responsibility of the testcase or the custom testcase to understand if there was an error in the rendering of the memraid.

I've moved the responsibility over to MermaidDiagram.

Copy link
Member

Choose a reason for hiding this comment

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

yeah agreed 💯, I was considering that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It got a lot better hahaha
done in 2e09454

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it in the custom test case so we could track the parseMermaid, not just the diagram error.

@ad1992
Copy link
Member

ad1992 commented May 1, 2024

Thanks, @igorwessel, I am doing the final review and making some tweaks. Will be pushing the changes soon so you can pause pushing for now to avoid conflicts :p

Add type attribute to each testcase
Use only activeTestCaseIndex and move to index.tsx to simplify and this also removes the hack to force rerender when render to excalidraw button clicked
@ad1992
Copy link
Member

ad1992 commented May 1, 2024

@igorwessel I have pushed some changes to simplify and clean up the code.
The important changes are in 55ac328 👇🏻

  • Add type attribute to each testcase so we need not pass name attribute and this could be helpful later
  • Use only activeTestCaseIndex and move one level up to index.tsx to simplify and this also removes the hack to force rerender when render to excalidraw button clicked
  • Remove isActiveCustomTest

Let me know if this looks good to you and if everything is working fine (I have tested it but another eye always helps :)). If all good we should be good to go :)

@igorwessel
Copy link
Contributor Author

@ad1992 Perfect, it looks great 💯 ! I had a somewhat incorrect mental model of this part from the beginning, it simplified quite a bit. I just pushed a small typo fix.

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

Thanks @igorwessel, merging 🚀

@ad1992 ad1992 merged commit e347696 into excalidraw:master May 2, 2024
2 checks passed
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.

Migrate playground build to Vite
2 participants