-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[WEB-2361] fix: Sidebar loosing its collapse context of on refresh and on changing the size of the window #5549
Conversation
Removed else statement which was expanding it whenever windowSize changed or webapp was hard refreshed.
WalkthroughThe change involves modifying the logic within the Changes
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 (
|
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
Outside diff range, codebase verification and nitpick comments (1)
web/app/[workspaceSlug]/(projects)/sidebar.tsx (1)
Line range hint
29-31
: Improved sidebar toggle logic.The modification to the
useEffect
hook ensures that the sidebar's state is maintained across window resizes and refreshes. This change aligns well with the PR objectives and should improve user experience by keeping the sidebar state consistent.However, it would be beneficial to add a comment explaining the logic behind this condition to aid future maintainability.
Consider adding a comment above the
useEffect
hook:// Toggle sidebar only when window size is less than 768px and sidebar is not already collapsed
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/app/[workspaceSlug]/(projects)/sidebar.tsx (1 hunks)
Additional comments not posted (1)
web/app/[workspaceSlug]/(projects)/sidebar.tsx (1)
Line range hint
23-27
: Review interaction between outside click detector and sidebar toggle logic.The
useOutsideClickDetector
is used to toggle the sidebar when a click is detected outside of it, under specific conditions. This logic appears to complement theuseEffect
changes. However, ensure that there are no conflicts or redundant toggles between this and theuseEffect
logic, especially during window resizing and sidebar interactions.Consider testing various scenarios where the sidebar might be interacted with, including rapid resizing and external clicks, to ensure the behavior remains consistent and as expected.
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
Outside diff range, codebase verification and nitpick comments (1)
web/app/[workspaceSlug]/(projects)/sidebar.tsx (1)
Line range hint
24-28
: Improved Logic in useEffect:The updated logic in the
useEffect
hook now correctly handles the sidebar's state by only toggling it when necessary. This should help maintain the sidebar's collapse state across different scenarios. It would be beneficial to add a comment explaining this logic for future maintainability.Consider adding a comment above the
useEffect
logic:// Toggle sidebar only when window size is less than 768px and sidebar is not already collapsed
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/app/[workspaceSlug]/(projects)/sidebar.tsx (1 hunks)
Additional comments not posted (3)
web/app/[workspaceSlug]/(projects)/sidebar.tsx (3)
Line range hint
1-1
: Review of Imports:The imports are well-organized and categorized, which enhances readability and maintainability. No issues found here.
Line range hint
18-22
: Clarification Needed on Sidebar Toggle Logic:The
useOutsideClickDetector
is used to toggle the sidebar when a click is detected outside of it. The condition checks if the sidebar is not collapsed and the window width is less than 768 pixels. Please clarify if this behavior is intended to only apply under these specific conditions, as it might not cover all scenarios where the sidebar should maintain its state.
Line range hint
30-60
: Well-Structured Component Rendering:The
AppSidebar
component is well-structured with clear conditional rendering based on thesidebarCollapsed
state. The use of thecn
helper function for managing class names is a good practice, enhancing the readability and maintainability of the component.
Problem Statement
Sidebar loosing its collapse context of on refresh and on changing the size of the window.
Solution
No context was being lost. There was if else statement which when the windows size changed or ran for first time, ran. The if else statement checked whether the window Size is less than 768px, the sidebar is to be collapsed else it should be expanded irrespective of value if it's previously collapsed or not.
Screenshots and Media
Before
Web.Application.-.Issues.-.Google.Chrome.2024-09-06.20-25-45.mp4
After
planedemo.-.Home.-.Google.Chrome.2024-09-06.20-30-39.mp4
References
[WEB-2361]
Summary by CodeRabbit
New Features
Bug Fixes