feat(ui): add burger menu toggle for collapsible sidebar with responsive and persistent behavior#767
Conversation
Reviewer's GuideThis PR adds a burger‐menu toggle to collapse and expand the left sidebar across desktop and mobile by injecting a custom toggle button into various navbar templates, overhauling the sidebar initialization and toggle logic in the main JS file (with localStorage persistence, responsive load/resize behavior, submenu hover and click enhancements), and introducing dedicated SCSS rules for minimized and mobile slide-in/out sidebar states. It also updates several template script includes and dependency versions. Sequence diagram for sidebar toggle interactionsequenceDiagram
actor User
participant BurgerMenu as Burger Menu Button
participant JS as Sidebar JS Logic
participant Sidebar
participant localStorage
User->>BurgerMenu: Clicks burger menu
BurgerMenu->>JS: Triggers click event
JS->>Sidebar: Toggle minimized/expanded state
JS->>localStorage: Save sidebarMinimized state
Sidebar-->>User: Sidebar animates (collapses/expands or slides in/out)
Class diagram for sidebar toggle JS logicclassDiagram
class SidebarToggle {
+applyInitialSidebarState()
+ensureMetisMenuInitialized()
+setupSidebarHover()
+isMobileView()
}
class localStorage {
+getItem(key)
+setItem(key, value)
}
class Body {
+addClass(className)
+removeClass(className)
+toggleClass(className)
}
SidebarToggle --|> localStorage : uses
SidebarToggle --|> Body : manipulates
SidebarToggle --|> Sidebar : toggles state
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
| <ul class="nav navbar-nav navbar-top-links navbar-left flip hidden-xs"> | ||
| {% if request.event %} | ||
| <li> | ||
| {% comment %} <div class="navbar-header"> |
There was a problem hiding this comment.
Please delete code, not comment, except when this change is temporary.
| {{ nav.icon|safe }} | ||
| {% endif %} | ||
| <a class="navbar-brand" href="{% url 'eventyay_common:dashboard' %}"> | ||
| <img src="{% static "pretixbase/img/eventyay-icon.svg" %}" /> |
There was a problem hiding this comment.
Please use different quote style when nesting.
| </form> | ||
| {% else %} | ||
| <a href="{% eventurl request.event "presale:event.index" %}" title="{% trans "View event" %}" target="_blank"> | ||
| <i class="fa fa-eye"></i> {% trans "View event" %} |
There was a problem hiding this comment.
Don't use the obsolete {% trans %} tag.
src/pretix/static/utils/js/utils.js
Outdated
| } | ||
| }); | ||
| } | ||
| } No newline at end of file |
|
|
||
| // Ensure the sidebar doesn't overlap with the navbar | ||
| .navbar-brand { | ||
| z-index: 1001; // Higher than sidebar |
There was a problem hiding this comment.
Why don't use z-index of 10 and 11? Why too high?
Gagan-Ram
left a comment
Gagan-Ram
left a comment
There was a problem hiding this comment.
767.webm
Hi @yaxit24, I noticed the following issues:
- The burger menu gets toggled off when navigating to a new page.
- The alignment of the burger menu items doesn't match that of ey-talk. Specifically, the top menu item icon appears to have extra padding and is aligned further left compared to the icons of the items below it.
- The entire Tickets component is displaying an "Internal Server Error".
- The burger menu icon is not visible on the Tickets component pages.
Also, please clarify why have you changed "Main dashboard" to "Tickets dashboard" and modified its links?
hongquan
left a comment
hongquan
left a comment
There was a problem hiding this comment.
You broke all quote styles.
The rule is to use different quote styles when nesting. Now you apply the same quote style everywhere!
| </form> | ||
| {% else %} | ||
| <a href="{% eventurl request.event 'presale:event.index' %}" title="{% translate "View event" %}" | ||
| <a href="{% eventurl request.event "presale:event.index" %}" title="{% trans "View event" %}" |
There was a problem hiding this comment.
Why you change the correct quote styles to wrong ones?
yaxit24
force-pushed
the
727-burger
branch
2 times, most recently
from
f8958fa to
0d7a0c1
Compare
|
please verify: Screen.Recording.2025-07-08.at.10.10.01.AM.mov |
Screen.Recording.2025-07-08.at.10.13.45.AM.mov |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
I dont understand the logic of the mobile view. Why should we have two burger menus in the mobile view? Please implement it in the same way as eventyay-talk. One mobile menu on the left is enough.
Thanks, implemented. I had added the functionality to both burger buttons to enhance accessibility on mobile devices, allowing them to open and close the sidebars from either side of the mobile view.
|
Gagan-Ram
left a comment
Gagan-Ram
left a comment
There was a problem hiding this comment.
- The alignment of the burger menu items doesn't match that of ey-talk. Specifically, the top menu item icon appears to have extra padding and is aligned further left compared to the icons of the items below it.
- If a sidebar menu item contains a sub-menu, selecting any sub-menu item will cause the entire dropdown menu to collapse.
mariobehling
changed the title
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please finalize the PR and make it against the enext branch.
|
As there were no updates on this PR I am closing it. |
…ive and persistent behavior
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
- The alignment of the burger menu items doesn't match that of ey-talk. Specifically, the top menu item icon appears to have extra padding and is aligned further left compared to the icons of the items below it.
- If a sidebar menu item contains a sub-menu, selecting any sub-menu item will cause the entire dropdown menu to collapse.
In the video it still looks like these points have not been resolved.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
app/eventyay/static/pretixcontrol/js/sb-admin-2.js:22
- Unused function isTabletView.
function isTabletView() {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // UNIVERSAL: Window resize handler | ||
| let resizeTimeout; | ||
| $(window).on('resize', function () { |
There was a problem hiding this comment.
It is safer to apply / unapply sidebar-minimized, sidebar-hover purely in CSS, using media queries, than JavaScript.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
app/eventyay/static/pretixcontrol/js/sb-admin-2.js:31
- Unused function isDesktopView.
function isDesktopView() {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| overflow-x: hidden; | ||
| scroll-behavior: smooth; | ||
| overscroll-behavior: contain; | ||
| contain: strict; |
There was a problem hiding this comment.
Using contain: strict can have unintended side effects as it applies layout, style, paint, and size containment. Consider using contain: layout style paint instead to avoid potential size containment issues that could affect the sidebar's ability to respond to content changes.
| contain: strict; | |
| contain: layout style paint; |
Screen.Recording.2025-11-06.at.12.58.42.AM.mov |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks, please address copilot reviews.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please address comments. Thanks.
Gagan-Ram
left a comment
Gagan-Ram
left a comment
There was a problem hiding this comment.
Works as expected.
Please address copilot suggestions.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| body.sidebar-minimized:has(.sidebar:hover) #page-wrapper { |
There was a problem hiding this comment.
The :has() pseudo-class has limited browser support in older browsers. While it's well-supported in modern browsers (Safari 15.4+, Chrome 105+, Firefox 121+), consider providing a fallback or ensuring this is acceptable for your target browser support matrix. Browsers that don't support :has() will not apply the margin adjustment when hovering over the minimized sidebar.
| body.sidebar-minimized:has(.sidebar:hover) #page-wrapper { | |
| // The :has() pseudo-class has limited browser support (Safari 15.4+, Chrome 105+, Firefox 121+). | |
| // For older browsers, use JS to add a .sidebar-hovered class to <body> when .sidebar is hovered. | |
| body.sidebar-minimized:has(.sidebar:hover) #page-wrapper, | |
| body.sidebar-minimized.sidebar-hovered #page-wrapper { |
| toggleSidebar(); const $navbar = $('.navbar'); | ||
| const $pageWrapper = $('#page-wrapper'); |
There was a problem hiding this comment.
Syntax error: There's a function call toggleSidebar(); placed in the middle of variable declarations. This call should be moved after all variable declarations are complete (after line 17 or later in the initialization flow).
| toggleSidebar(); const $navbar = $('.navbar'); | |
| const $pageWrapper = $('#page-wrapper'); | |
| const $navbar = $('.navbar'); | |
| const $pageWrapper = $('#page-wrapper'); | |
| toggleSidebar(); |
| function updateCSSVariables() { | ||
| document.documentElement.style.setProperty('--navbar-height', getNavbarHeight() + 'px'); | ||
| } | ||
| const $sidebarToggleButton = $('#sidebar-toggle'); |
There was a problem hiding this comment.
The $sidebarToggleButton variable is declared after the updateCSSVariables() function definition but before it's needed. Consider moving this declaration to be with the other variable declarations at the top (lines 14-17) for better code organization and readability.
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please address the remaining open comments in this PR. Thanks
Removed whitespaces. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ive and persistent behavior (fossasia#767) * feat(ui): add burger menu toggle for collapsible sidebar with responsive and persistent behavior * Updated with cps-decorators * fixed ai reviews * Edited files: admin.py, cfp.py, event.py, mixins.py * Reverted changes in eventyay.cfg * Remove unnecessary newline at end of mixins.py * updated the eventyay.cfg * solved ai reviews, added scrolling to sidebar. * reverted app/eventyay.cfg modifications * Improved code quality * Update app/eventyay/static/pretixcontrol/scss/main.scss Removed whitespaces. --------- Co-authored-by: Mario Behling <mb@mariobehling.de>
Fixes: #727
Screen.Recording.2025-06-27.at.4.22.52.PM.mov
Summary by Sourcery
Implement a burger menu toggle to collapse and expand the left sidebar with responsive behavior and persistent state across desktop and mobile.
Enhancements: