refactor: remove jQuery from circuit.js and use Vue reactivity #575
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis change removes remaining jQuery-based UI event handling and DOM manipulation code from the circuit management logic, replacing it with comments noting migration to Vue components and reactive state updates. The update clarifies the separation of UI logic from core circuit logic, with no changes to public function signatures or core data structures. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
v1/src/simulator/src/circuit.js (1)
271-282: Consider removing commented line for cleaner code.The commented line about
activeCircuit.value.nameshould either be implemented if needed or removed to keep the code clean.const index = circuit_list.value.findIndex((circuit) => circuit.id === id) circuit_list.value[index].name = name - // activeCircuit.value.name = name // add later if necessary at current stage not important handled by projectProperty on switching circuit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v1/src/simulator/src/circuit.js(6 hunks)
🔇 Additional comments (3)
v1/src/simulator/src/circuit.js (3)
87-90: Excellent migration to Vue reactivity!The replacement of jQuery class toggling with Vue reactive state updates is well-implemented. The code correctly clears the
focussedstate of all circuits before setting the new focused circuit, maintaining the same logical behavior while leveraging Vue's reactivity system.
236-249: Clear documentation of Vue migration.The updated comments effectively document the migration of jQuery event handlers to Vue components. This provides good context for future developers understanding why certain jQuery code is commented out.
206-210:✅ Verification successful
Verify Verilog state management logic.
The comment indicates "Reset all other circuits' verilog state" but the code sets
isVerilog = falsefor all circuits, then immediately sets the last circuit'sisVerilog = true. This seems to implement exclusive Verilog state (only one circuit can be Verilog at a time).Please verify this is the intended behavior. If multiple circuits should be able to have Verilog state simultaneously, this logic needs adjustment:
🏁 Script executed:
#!/bin/bash # Search for other references to isVerilog property to understand intended behavior rg -A 3 -B 3 "isVerilog.*=" --type jsLength of output: 8977
Verilog state exclusivity confirmed.
The
newCircuitfunction in v1/src/simulator/src/circuit.js (lines 206–210) deliberately clears every circuit’sisVerilogflag then sets it only on the newly added/loaded circuit, matching the// Reset all other circuits' verilog statecomment. This enforces a single Verilog circuit at a time.• v1/src/simulator/src/circuit.js:206–210 – reset loop enforces exclusive
isVerilogstateIf you intend to allow multiple circuits to carry Verilog state simultaneously, you’ll need to remove or adjust that reset loop.
|
@Arnabdaz @niladrix719 can you pls review this pr , thanks |
|
Thanks for the PR but we don't want ts integration in v0 or v1 at the moment, You can try it for files which dont have pre-existing PRs |
Fixes #433
Describe the changes you have made in this PR -
Removed remaining jQuery from circuit.js and migrated DOM logic to Vue reactivity.
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit