Collapsible menu

[bengtmagnus]

WHAT is this pull request doing?

Makes it possible to switch between expanded and collapsed sidebar menu. When collapsed, only the icons are visible with a clear hover indicator on which page is applicable.

A variable is saved in LocalStorage to see if the menu should be collapsed or expanded when the page loads. This check is done directly in the head to avoid jumping. The collapsed/expanded class is set on the html tag.

Since we want overflow auto in the y-direction but hidden in the x-direction on the menu, the tooltips for the menu options need to be fixed. That's why I calculate the position with javascript.

HOW can this pull request be tested?

Switch between the different states

[claude]

Claude finished @bengtmagnus's task —— View job

---

Code Review Results

Reviewing collapsible menu implementation for issues:

• Read and analyze layout.js changes
• Review main.css modifications
• Check head.ecr template changes
• Analyze header.ecr modifications
• Report findings (bugs, security, performance, error handling, Crystal anti-patterns only)

Issues Found

Missing Error Handling:

1. layout.js:149-164 - Missing null checks for DOM elements:

   const sidebar = document.getElementById('menu-content');
   const menuItems = document.querySelectorAll('#menu-content li a.menu-tooltip');

   If #menu-content doesn't exist, sidebar will be null and the code will fail.

2. layout.js:167-174 - Missing null check for #menu element:

   const sidebarMenu = document.getElementById('menu');
   sidebarMenu.addEventListener('scroll', function() {

   If #menu doesn't exist, this will throw an error.

Performance Issues:

3. layout.js:169-173 - Inefficient scroll handler that runs on every scroll event without throttling:

   sidebarMenu.addEventListener('scroll', function() {
     menuItems.forEach(item => {
       const tooltip = item.querySelector('.menu-tooltip-label');
       const rect = item.getBoundingClientRect();
       tooltip.style.top = rect.top + (rect.height / 2) + 'px';
     });
   });

   This should be throttled/debounced to avoid excessive DOM manipulation.

Bug:

4. layout.js:154 - Missing null check for tooltip element:

   const tooltip = item.querySelector('.menu-tooltip-label');

   If tooltip doesn't exist, the code will fail when trying to set tooltip.style.left.

[carlhoerberg]

Looks great! But clicking the username, that's only temporary?

[bengtmagnus]

Looks great! But clicking the username, that's only temporary?

Ah! yes :) Will add a proper button for it. Just wanted to test the toggle on click.

[spuun]

I pushed a fix for the CSP header.

[ThomasSarlin]

Looks really good and works well. Tried experimenting with a way to implement this without having to add the scroll event. The behavior is possible with pure css, but it got out of hand and ended up being a messier restructure of the whole page.

We could improve and prevent sending scroll events (and in turn update the css top variable for non-collapsed labels) when the menu is not collapsed but current implementation does not affect the page visually so it might be unnecessary.