-
Notifications
You must be signed in to change notification settings - Fork 133
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
refactor : ts integration : src/simulator/src/tutorials.ts #445
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request updates TypeScript usage in the simulator’s tutorial system. In the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DOC as DOM
participant TW as tutorialWrapper
U->>DOC: Query for panelHeader element
TW->>DOC: Attaches event listener (once)
Note right of DOC: Listener triggers only once
sequenceDiagram
participant TG as showTourGuide
participant MB as MaximizeButton Element
TG->>MB: Invokes click (if element exists via optional chaining)
Assessment against linked issues
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/simulator/src/types/tutorial.types.ts (1)
1-11
: Add JSDoc documentation to improve code clarity.The interface is well-structured, but adding JSDoc documentation would improve code clarity and maintainability.
Add documentation like this:
+/** + * Represents a step in the tutorial tour. + */ export interface TourStep { + /** CSS selector for the element to highlight */ element: string; + /** Optional CSS class to apply to the highlighted element */ className?: string; + /** Configuration for the popover tooltip */ popover: { + /** Optional CSS class to apply to the popover */ className?: string; + /** Title text displayed in the popover */ title: string; + /** Detailed description displayed in the popover */ description: string; + /** Position of the popover relative to the highlighted element */ position: 'left' | 'right' | 'top' | 'bottom'; + /** Optional offset in pixels from the default position */ offset?: number; }; }src/simulator/src/tutorials.ts (2)
84-104
: Improve type assertions in event handler.While the optional chaining is good, the type assertions in the click handler could be improved for better type safety.
Consider this improvement:
- document.querySelector('.panelHeader')?.addEventListener('click', (e) => { + document.querySelector<HTMLElement>('.panelHeader')?.addEventListener('click', (e: MouseEvent) => { if (localStorage.tutorials === 'next') { panelHighlight.highlight({ element: '#guide_1', popover: { title: 'Here are the elements', description: 'Select any element by clicking on it & then click anywhere on the grid to place the element.', position: 'right', - offset: ((e.target as HTMLElement).nextElementSibling as HTMLElement)?.offsetHeight + (e.target as HTMLElement).offsetTop - 45, + offset: (e.currentTarget as HTMLElement).nextElementSibling?.offsetHeight + (e.currentTarget as HTMLElement).offsetTop - 45, }, }); localStorage.setItem('tutorials', 'done'); } }, { once: true });Changes:
- Use
querySelector<HTMLElement>
to get better type inference- Type the event parameter as
MouseEvent
- Use
currentTarget
instead oftarget
as it's more reliable in event delegation
113-118
: Improve type assertion and localStorage consistency.Two suggestions for improvement:
- Use
querySelector<HTMLElement>
for better type inference- Consider using a constant for localStorage keys to maintain consistency
Consider these improvements:
+const STORAGE_KEYS = { + TUTORIAL_STATUS: 'tutorials', + TUTORIAL_TOUR_DONE: 'tutorials_tour_done' +} as const; export function showTourGuide(): void { - (document.querySelector('.draggable-panel .maximize') as HTMLElement)?.click(); + document.querySelector<HTMLElement>('.draggable-panel .maximize')?.click(); animatedTourDriver.defineSteps(tour); animatedTourDriver.start(); - localStorage.setItem('tutorials_tour_done', 'true'); + localStorage.setItem(STORAGE_KEYS.TUTORIAL_TOUR_DONE, 'true'); }Update the
tutorialWrapper
function to use the constant:- if (localStorage.tutorials === 'next') { + if (localStorage.getItem(STORAGE_KEYS.TUTORIAL_STATUS) === 'next') { // ... - localStorage.setItem('tutorials', 'done'); + localStorage.setItem(STORAGE_KEYS.TUTORIAL_STATUS, 'done'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/tutorials.ts
(8 hunks)src/simulator/src/types/tutorial.types.ts
(1 hunks)
🔇 Additional comments (1)
src/simulator/src/tutorials.ts (1)
4-81
: LGTM! Tour configuration is well-structured.The tour configuration is properly typed as
TourStep[]
and the descriptions are clear and informative.
Fixes #414
@JoshVarga @Arnabdaz @niladrix719 @devartstar
Summary by CodeRabbit