feat(circuit): Added labels for circuit elements in Simulator #405
Conversation
WalkthroughThe changes modify the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/Panels/ElementsPanel/ElementsPanel.vue (2)
128-130: Enhance text handling and accessibility for element namesWhile the basic structure works, there are opportunities to improve the text handling and accessibility:
- Add text truncation with ellipsis for long names
- Show full name in tooltip when truncated
- Improve screen reader support
Apply these improvements:
- <div class="overflow-hidden text-nowrap position-relative"> - <p class="d-inline-block">{{ element.name }}</p> + <div class="overflow-hidden text-nowrap position-relative" style="max-width: 100%;"> + <p class="d-inline-block text-truncate" style="max-width: 100%;" :title="element.name" aria-label="Circuit element name: ${element.name}"> + {{ element.name }} + </p> </div>
Line range hint
1-131: Consider extracting element display into a reusable componentTo maintain consistency and reduce code duplication, consider extracting the element display structure (image + name) into a separate component that can be reused across all three locations: main panel, search results, and category search results.
This would:
- Ensure consistent display across all views
- Make future updates easier to maintain
- Reduce code duplication
- Centralize styling and accessibility improvements
| @@ -40,6 +40,7 @@ | |||
| @mouseleave="tooltipText = 'null'" | |||
| > | |||
| <img :src="element.imgURL" :alt="element.name" /> | |||
|
|
|||
There was a problem hiding this comment.
Add element name div to search results for consistency
The element names are only shown in the expansion panels but not in the search results. This creates an inconsistent user experience. To align with the PR objective of enhancing clarity, add the same name display structure here.
Add this after the img element:
<img :src="element.imgURL" :alt="element.name" />
+ <div class="overflow-hidden text-nowrap position-relative">
+ <p class="d-inline-block">{{ element.name }}</p>
+ </div>Committable suggestion skipped: line range outside the PR's diff.
| @@ -77,6 +78,7 @@ | |||
| :src="element.imgURL" | |||
| :alt="element.name" | |||
| /> | |||
|
|
|||
There was a problem hiding this comment.
Add element name div to category search results for consistency
The element names are missing in the category search results as well. This should be consistent with the main panel view.
Add this after the img element:
<img :src="element.imgURL" :alt="element.name" />
+ <div class="overflow-hidden text-nowrap position-relative">
+ <p class="d-inline-block">{{ element.name }}</p>
+ </div>Committable suggestion skipped: line range outside the PR's diff.
nickhil-verma
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/simulator/src/data/project.ts (1)
158-167: Simplify the complex boolean condition.The nested boolean logic can be simplified for better readability:
- if ( - !verify && - (!projectSaved && checkToSave()) && - !(await confirmOption( - 'What you like to start a new project? Any unsaved changes will be lost.' - )) - ) { + const hasUnsavedChanges = !projectSaved && checkToSave(); + if (!verify && hasUnsavedChanges) { + const confirmed = await confirmOption( + 'Would you like to start a new project? Any unsaved changes will be lost.' + ); + if (!confirmed) { + return; + } + }Also, there's a typo in the confirmation message: "What you like" should be "Would you like".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/data/project.ts(2 hunks)
🔇 Additional comments (2)
src/simulator/src/data/project.ts (2)
123-139:⚠️ Potential issueVerify if removing the unsaved changes warning is intentional.
Commenting out the
window.onbeforeunloadhandler removes critical user experience safeguards:
- Users won't be warned when navigating away with unsaved changes
- Increased risk of accidental data loss
- Deviates from standard web application behavior
While the recovery data is still saved (lines 134-136), users aren't informed about this safety net.
Let's verify if this change aligns with other navigation-related code:
✅ Verification successful
The commented code appears to be an intentional simplification
The change maintains the essential unsaved changes protection:
- The browser's native "Leave Site?" dialog is still triggered by the return statement
- The auto-recovery mechanism via localStorage remains active
- Protection for new project creation is still in place
The modification simply removes a redundant custom alert while keeping the standard browser behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other navigation handlers or save prompts rg -g '*.{ts,vue}' -A 3 'onbeforeunload|beforeunload|navigationGuard|router.beforeEach' # Look for related user experience patterns ast-grep --pattern 'confirmOption($$$unsaved$$$)'Length of output: 400
Script:
#!/bin/bash # Search for Vue router guards and navigation protection rg -g '*.{ts,vue}' -A 5 'router\.beforeEach|beforeRouteLeave|navigation-guard' # Look for save-related dialogs or confirmations rg -g '*.{ts,vue}' -A 3 'confirm.*save|unsaved.*changes|save.*warning' # Check for recent changes to navigation-related files git diff HEAD~10 -- '**/router*' '**/navigation*' # Search for alternative dialog implementations ast-grep --pattern 'confirm($$$save$$$)'Length of output: 1750
173-174: Review the URL construction logic.The current implementation has several concerns:
- Hardcoded fallback to 'localhost:4000' might not be suitable for all environments
- Hardcoded path '/simulatorvue/' might cause issues if the application is deployed under a different path
Consider:
- Using environment variables for the base URL and path
- Using the router's configuration for navigation instead of direct URL manipulation
Let's verify the deployment configurations:
|
can you review this PR @tachyons |
Fixes #406
I've made changes to improve the clarity of circuit diagrams in the project, especially in the "misc" section. Previously, these diagrams were presented without labels or names for the circuit elements, making it difficult for newcomers to understand them.
Changes:- Added labels or names for the elements directly below the corresponding diagrams or images. This will make it easier for users to identify and understand the components used in the circuits.
Screenshots of Before changes -
Screenshots of Before changes -
Summary by CodeRabbit
Summary by CodeRabbit
UI Improvements
User Interaction Changes