-
Notifications
You must be signed in to change notification settings - Fork 134
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 Codebase: Replace jQuery with Vue.js in Key Components #465
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis change introduces a new print feature for Boolean logic tables in the combinational analysis component. The new printBooleanTable() function creates a styled table in a new window and initiates printing, while the createBooleanPrompt() function is refactored for clearer input handling. Additionally, the theme component now uses Vue’s reactive paradigm by replacing jQuery-based DOM manipulations with reactive references and object-based class binding, streamlining theme selection and state management. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CA as CombinationalAnalysis Component
participant Win as New Window
User->>CA: Initiate print table operation
CA->>CA: Generate table header and body
CA->>Win: Open new window with generated HTML table
Win->>User: Display table and trigger print dialog
sequenceDiagram
participant User as User
participant AT as ApplyThemes Component
participant LS as LocalStorage
User->>AT: Select theme
AT->>AT: Update selectedThemeRef reactively
AT->>AT: Apply 'selected' and 'set' classes using object syntax
AT->>LS: Save updated theme setting
Possibly related issues
Poem
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: 3
🧹 Nitpick comments (6)
src/components/DialogBox/Themes/ApplyThemes.vue (3)
29-32
: Consider consolidating theme state tracking.The class binding uses two separate refs (
selectedTheme
andselectedThemeRef
) to track theme state, which could lead to confusion. Consider consolidating these into a single reactive reference with an enum or string constant to represent different states (e.g., 'selected', 'applied').-:class="{ - selected: theme === selectedTheme, - set: theme === selectedThemeRef -}" +const ThemeState = { + SELECTED: 'selected', + APPLIED: 'applied' +} as const + +:class="{ + [ThemeState.SELECTED]: theme === currentTheme?.selected, + [ThemeState.APPLIED]: theme === currentTheme?.applied +}"
216-226
: Consolidate theme storage logic.The theme storage logic is duplicated between
applyTheme
andapplyCustomTheme
. Consider extracting this into a reusable function.+function saveThemeToStorage(theme: string, customThemeData?: object) { + try { + localStorage.setItem('theme', theme) + if (customThemeData) { + localStorage.setItem('Custom Theme', JSON.stringify(customThemeData)) + } + } catch (error) { + console.error('Failed to save theme:', error) + // Handle the error appropriately + } +} function applyCustomTheme() { iscustomTheme.value = true updateThemeForStyle(localStorage.getItem('theme')) updateBG() - localStorage.setItem('theme', 'Custom Theme') - localStorage.setItem( - 'Custom Theme', - JSON.stringify(themeOptions['Custom Theme']) - ) + saveThemeToStorage('Custom Theme', themeOptions['Custom Theme']) selectedThemeRef.value = selectedTheme.value }
1-297
: Overall implementation successfully migrates from jQuery to Vue.js.The changes effectively leverage Vue.js features like reactive refs and class binding, aligning well with the PR objectives. The removal of jQuery dependencies improves the codebase's consistency and maintainability.
However, consider these architectural improvements:
- Use Vue's
computed
properties for derived state- Implement proper error boundaries
- Consider using Vue's Composition API features like
provide/inject
for theme managementsrc/components/DialogBox/CombinationalAnalysis.vue (3)
220-220
: Remove unused 'index' parameter.The
index
parameter in the map function is not being used.Apply this diff:
- tableContent.innerHTML = tableHeader.value.map((header, index) => ` + tableContent.innerHTML = tableHeader.value.map(header => `🧰 Tools
🪛 ESLint
[error] 220-220: 'index' is defined but never used.
(no-unused-vars)
262-262
: Remove unused 'scope' parameter.The
scope
parameter is assigned a default value but never used in the function.Apply this diff:
-function createBooleanPrompt(inputList, outputList, scope = globalScope) { +function createBooleanPrompt(inputList, outputList) {🧰 Tools
🪛 ESLint
[error] 262-262: 'scope' is assigned a value but never used.
(no-unused-vars)
342-342
: Remove commented CSS rule.The commented out
top: 50%
rule should be removed as it's not being used.Apply this diff:
.alertStyle { position: absolute; - /* top: 50%; */ top: 100px;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/DialogBox/CombinationalAnalysis.vue
(1 hunks)src/components/DialogBox/Themes/ApplyThemes.vue
(3 hunks)
🧰 Additional context used
🪛 ESLint
src/components/DialogBox/CombinationalAnalysis.vue
[error] 220-220: 'index' is defined but never used.
(no-unused-vars)
[error] 262-262: 'scope' is assigned a value but never used.
(no-unused-vars)
🔇 Additional comments (2)
src/components/DialogBox/CombinationalAnalysis.vue (2)
1-20
: LGTM! Template section follows Vue.js best practices.The template section properly utilizes Vue's reactivity system with
v-model
, event bindings, and conditional rendering.
263-323
: LGTM! Modern JavaScript practices implemented.The refactored code successfully:
- Uses optional chaining for null safety
- Implements modern array methods
- Uses template literals
- Follows Vue.js reactivity patterns
Hi @ThatDeparted2061 , The issue was already assigned to me on Slack before you raised the PR, and since I was already working on it, I submitted my PR. |
Hey riddhi, There is no assignment to this issue because this is intened to be done by everyone who wants, to speed things up. |
@rxiddhi @ThatDeparted2061 I think the simpler thing to do here would be to create two separate issues focusing on separate set of files and work on them. |
I dont think there is a need for that, |
Fixes # 433
Describe the changes you have made in this PR -
Migrated from jQuery to Vue.js Native Methods:
All jQuery selectors like $('.messageBox .v-card-text') have been replaced with Vue’s refs for direct DOM manipulation.
Replaced jQuery event handling methods (e.g., $('.button').on('click', ...)) with Vue’s @click, @input, and other event listeners that integrate with Vue’s reactivity system.
Refactored the truth table generation and Boolean function manipulation logic to use Vue's ref and reactive for managing the data, eliminating the need for jQuery.
Leveraged Vue’s Reactive System:
State management is now handled using Vue’s reactive data (ref and reactive), replacing the previous jQuery-based state handling.
The generation and display of the table content have been refactored to use Vue’s templating system and list rendering (v-for), making the code more efficient and easier to maintain.
Replaced jQuery’s Iteration Methods:
Instead of using jQuery's $.each for looping through arrays, I’ve switched to using JavaScript’s modern array methods like map, forEach, and join.
Improved Functionality:
The printBooleanTable function and the overall table creation process have been refactored to make better use of Vue's built-in features.
Instead of using jQuery for dynamic DOM updates, Vue’s v-if and v-for directives are now used to render table rows and headers based on the data.
Code Modernization:
Replaced traditional for loops with more modern for...of loops and used template literals for better readability and performance.
In ApplyThemes.vue, I’ve optimized the theme handling logic by removing jQuery's class manipulation methods and replacing them w
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit