Make left side bar move in/out #192
Conversation
Reviewer's Guide by SourceryThis pull request refactors the sidebar functionality to allow it to move in and out using a button. Key changes include adding ref attributes to key elements, updating the toggleSidebar method to manipulate styles directly, and modifying CSS for smooth transitions. Additionally, several components were updated to ensure full height layout consistency. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using CSS classes instead of inline styles for toggling sidebar visibility. This would improve maintainability and separation of concerns.
- Replace hard-coded values (e.g., '280px') with CSS variables for better consistency and easier future modifications.
- Remove commented-out code that is no longer needed to improve code cleanliness.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -151,7 +156,21 @@ export default { | |||
| this.$store.dispatch('notifications/clearDesktopNotifications') | |||
| }, | |||
| toggleSidebar () { | |||
| this.showSidebar = !this.showSidebar | |||
| if(this.$refs.roomsSidebar && this.$refs.roomsSidebar.$el) { | |||
| this.$refs.roomsSidebar.$el.style.width = !!this.$refs.roomsSidebar.$el.offsetWidth ? '0px' : '280px'; | |||
There was a problem hiding this comment.
suggestion: Consider using CSS variables instead of magic numbers
Replace hard-coded values like '280px' with CSS variables. This will improve maintainability and make it easier to adjust these values globally.
this.$refs.roomsSidebar.$el.style.width = !!this.$refs.roomsSidebar.$el.offsetWidth ? '0px' : 'var(--sidebar-width, 280px)';
webapp/src/components/Scrollbars.vue
Outdated
| @@ -210,6 +210,7 @@ $thumb-width-hovered = 12px | |||
| box-sizing: border-box | |||
| min-height: 0 | |||
| flex: auto | |||
| height: 100vh | |||
There was a problem hiding this comment.
issue (bug_risk): Be cautious with 100vh on mobile browsers
Using 100vh can cause issues on mobile browsers where the viewport height changes when the address bar appears/disappears. Consider using a more robust solution, such as the '100dvh' unit or a JavaScript-based approach.
|
Yes, the moving in of the sidebar looks good, but how to handle the mobile view? This breaks the mobile view. This PR: Current view on production: |
|
I think we could keep the way it currently works on mobile. |
Summary by Sourcery
Implement a toggle functionality for the left sidebar, allowing it to move in and out with smooth transitions. Refactor the layout to use fixed positioning for the sidebar and adjust the main content area accordingly.
Enhancements: