feat: Introduce <ToggleIconButton/> for single toggles and replace <ToggleButtonGroup/>.
#191
Conversation
WalkthroughThis update modifies both the styling and state management within the Changes
Possibly related PRs
Suggested reviewers
✨ 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 (
|
Henry8192
changed the title
ToggleButtonGroup with Stack for query option icon buttons
![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)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
68-82: Consider optimizing the button handlers.The handlers follow a consistent pattern but could be optimized to avoid redundant state updates.
Consider this optimization:
- const handleCaseButtonPressed = () => { - handleQuerySubmit({ - isCaseSensitive: !isCaseSensitive, - isRegex: isRegex, - }); - setIsCaseSensitive(!isCaseSensitive); - }; + const handleCaseButtonPressed = () => { + const newIsCaseSensitive = !isCaseSensitive; + setIsCaseSensitive(newIsCaseSensitive); + handleQuerySubmit({ + isCaseSensitive: newIsCaseSensitive, + isRegex: isRegex, + }); + }; - const handleRegexButtonPressed = () => { - handleQuerySubmit({ - isCaseSensitive: isCaseSensitive, - isRegex: !isRegex, - }); - setIsRegex(!isRegex); - }; + const handleRegexButtonPressed = () => { + const newIsRegex = !isRegex; + setIsRegex(newIsRegex); + handleQuerySubmit({ + isCaseSensitive: isCaseSensitive, + isRegex: newIsRegex, + }); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🧠 Learnings (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css (1)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css:14-18
Timestamp: 2024-11-14T04:35:00.986Z
Learning: In `src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css`, the use of `!important` flags is necessary for overriding styles in the `.query-input-box` class.
🔇 Additional comments (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css (1)
20-24: LGTM! The button dimensions are appropriate.The explicit dimensions with
!importantflags are necessary and appropriate for icon buttons. Setting both fixed and minimum dimensions ensures consistent button sizing.src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (4)
11-11: LGTM! The Stack import is appropriate.The Stack component is a suitable replacement for ToggleButtonGroup to manage button layout.
47-48: LGTM! The state management is simplified.Using boolean flags for case sensitivity and regex options is more straightforward than the previous enum-based approach.
56-58: LGTM! The query submission is simplified.Direct use of boolean flags makes the code more readable and maintainable by removing the need for utility functions.
111-148: LGTM! The UI changes are well-implemented.The Stack component with IconButtons provides a clean and accessible replacement for the toggle button group. The buttons are properly configured with tooltips, aria attributes, and consistent styling.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (3)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx (1)
1-44: Well-implemented component with good accessibility practices!This new
ToggleIconButtoncomponent is a well-structured solution that provides the toggle functionality needed for the query options. I especially appreciate:
- The clear TypeScript interface extending the base IconButtonProps
- The use of
aria-pressedattribute for accessibility- The thorough JSDoc documentation
- The component composition with Tooltip for better UX
This implementation aligns well with the PR objective of simplifying the query option state management.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
45-46: Great simplification of state managementThe replacement of the enum-based approach with direct boolean state variables is a good improvement. This makes the code more maintainable and easier to understand.
The state variables have clear, descriptive names, and they're used consistently throughout the component.
Also applies to: 54-56
109-132: Good UI structure with Stack and ToggleIconButtonThe replacement of ToggleButtonGroup with Stack and custom ToggleIconButton components creates a cleaner implementation that more directly manages the toggle states.
The props passed to ToggleIconButton are well-structured and include all necessary information including accessibility attributes and tooltips.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
|
For the PR title, how about: |
Henry8192
changed the title
ToggleButtonGroup with Stack for query option icon buttons<ToggleIconButton/> for single toggles and replace <ToggleButtonGroup/>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
67-72:⚠️ Potential issueRace condition in case sensitivity toggle handler.
The function submits the query using the current state value and then updates the state. This creates a race condition where the query might not use the toggled value.
Apply this fix:
const handleCaseSensitivityButtonClick = () => { + const newCaseSensitiveValue = !isCaseSensitive; + setIsCaseSensitive(newCaseSensitiveValue); handleQuerySubmit({ - isCaseSensitive: isCaseSensitive, + isCaseSensitive: newCaseSensitiveValue, }); - setIsCaseSensitive(!isCaseSensitive); };
74-79:⚠️ Potential issueRace condition in regex toggle handler.
Similar to the case sensitivity handler, this function has a race condition where the query is submitted before the state is updated.
Apply this fix:
const handleRegexButtonClick = () => { + const newRegexValue = !isRegex; + setIsRegex(newRegexValue); handleQuerySubmit({ - isRegex: !isRegex, + isRegex: newRegexValue, }); - setIsRegex(!isRegex); };
🧹 Nitpick comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
69-70: Redundant property in query submission.Setting
isCaseSensitive: isCaseSensitiveis redundant as this value is already set in the base query args at line 55.Remove the unnecessary property:
const handleCaseSensitivityButtonClick = () => { - handleQuerySubmit({ - isCaseSensitive: isCaseSensitive, - }); + handleQuerySubmit({}); setIsCaseSensitive(!isCaseSensitive); };
76-76: Coding guideline: Use of negation operator.According to the coding guidelines provided, we should prefer
false == <expression>rather than!<expression>.handleQuerySubmit({ - isRegex: !isRegex, + isRegex: false == isRegex, });Note: There was a previous discussion in comments where the team decided to keep using
!for readability in this specific case. If that decision still stands, you can ignore this comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (4)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (4)
10-10: LGTM: Import changes for new component structure.The addition of
Stackfrom MUI Joy and the newToggleIconButtoncomponent follows the PR's objective to replace the previousToggleButtonGroupimplementation.Also applies to: 31-31
46-47: LGTM: State variables for direct query option management.The implementation of dedicated state variables for case sensitivity and regex options improves clarity compared to the previous enum-based approach.
55-56: LGTM: Updated query submission with new state variables.The query submission handler correctly uses the new state variables and the spread operator to override with any new values.
Also applies to: 58-58
108-135: LGTM: Improved UI with dedicated toggle buttons.The new UI implementation with
StackandToggleIconButtoncomponents provides a cleaner and more maintainable solution than the previous implementation. Each toggle button now has clear tooltips and visual feedback.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
67-72:⚠️ Potential issueFix potential race condition and follow coding guidelines
The toggle handler has two issues:
- It submits the query before updating the state, which could lead to a race condition where the query uses the old state value.
- The negation operator (
!isCaseSensitive) doesn't follow the coding guideline that prefersfalse == <expression>over!<expression>.Apply this fix:
const handleCaseSensitivityButtonClick = () => { + const newCaseSensitiveValue = false == isCaseSensitive; + setIsCaseSensitive(newCaseSensitiveValue); handleQuerySubmit({ - isCaseSensitive: !isCaseSensitive, + isCaseSensitive: newCaseSensitiveValue, }); - setIsCaseSensitive(!isCaseSensitive); };
74-79:⚠️ Potential issueFix potential race condition and follow coding guidelines
The regex toggle handler has the same issues as the case sensitivity handler:
- It submits the query before updating the state, which could lead to a race condition.
- The negation operator (
!isRegex) doesn't follow the coding guideline that prefersfalse == <expression>over!<expression>.Apply this fix:
const handleRegexButtonClick = () => { + const newRegexValue = false == isRegex; + setIsRegex(newRegexValue); handleQuerySubmit({ - isRegex: !isRegex, + isRegex: newRegexValue, }); - setIsRegex(!isRegex); };
🧹 Nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
41-41: Consider refactoring to reduce function sizeThe ESLint disable comment indicates the function exceeds the line limit. Consider if some logic could be extracted into separate helper functions to improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ToggleIconButton.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
108-135: UI update looks goodThe replacement of ToggleButtonGroup with a Stack containing ToggleIconButton components is well implemented. Each button properly passes the isChecked state and has appropriate tooltip titles.
46-47: Good state initializationThe new state variables are correctly initialized to
falseas default values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
66-69: Consider updating state before submitting to prevent potential race conditionsThe current implementation submits the query before updating the state. This could potentially lead to race conditions where the state update isn't completed before the next render cycle.
Consider reversing the order of operations:
const handleCaseSensitivityButtonClick = () => { + setIsCaseSensitive(!isCaseSensitive); handleQuerySubmit({isCaseSensitive: !isCaseSensitive}); - setIsCaseSensitive(!isCaseSensitive); };
71-74: Consider updating state before submitting to prevent potential race conditionsSimilar to the case sensitivity handler, this function submits the query before updating the state, which could lead to potential race conditions.
Consider reversing the order of operations:
const handleRegexButtonClick = () => { + setIsRegex(!isRegex); handleQuerySubmit({isRegex: !isRegex}); - setIsRegex(!isRegex); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (5)
67-67: Consider usingfalse == expressionpattern per coding guidelinesThe negation operator usage (
!isCaseSensitive) doesn't follow the coding guideline that suggests usingfalse == <expression>instead of!<expression>.Note: I see from past comments that the team had a discussion about this and decided to use
!for better readability in this particular case. If that decision still holds, this can be ignored.
72-72: Consider usingfalse == expressionpattern per coding guidelinesThe negation operator usage (
!isRegex) doesn't follow the coding guideline that suggests usingfalse == <expression>instead of!<expression>.Note: Based on past comments, the team had discussed this and decided to use
!for better readability in this context. If that decision still holds, this can be ignored.
103-130: Great UI improvement with the new ToggleIconButton componentThe replacement of the previous toggle button implementation with the new
ToggleIconButtoncomponent creates a cleaner UI and better separation of concerns. The Stack layout also provides a more consistent spacing solution.
45-46: Good state management updateMoving to direct state variables for
isCaseSensitiveandisRegexsimplifies the code and makes state management more straightforward than the previous enum-based approach.
54-56: Solid update to the query submission logicThe updated query submission function properly utilizes the new state variables, making the code more maintainable and reducing complexity.
Description
ToggleButtonGroupis causing inconvenience wheneverisCaseSensitiveorisRegexupdates. It is only used because the button pressed status needs to be shown visually. The current implementation achieves the same goal without usingToggleButtonGroup, and are easier for query options to update.Checklist
breaking change.
Validation performed
Search
Schedulingwithout any query options, search results are found and shown;Enable case sensitive, no search results are found.
example.zip
Summary by CodeRabbit
New Features
ToggleIconButtoncomponent for enhanced user interaction with search options.Refactor
Style