Skip to content

Hide django sidebar by default in recap screen#4556

Merged
patrick91 merged 1 commit intomainfrom
hide-sidebar-in-recap
Feb 4, 2026
Merged

Hide django sidebar by default in recap screen#4556
patrick91 merged 1 commit intomainfrom
hide-sidebar-in-recap

Conversation

@patrick91
Copy link
Member

@patrick91 patrick91 commented Feb 4, 2026

This changes the local storage, but hopefully it's not a big deal to have the sidebar closed by default after opening the recap pages

@vercel
Copy link

vercel bot commented Feb 4, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
pycon Ready Ready Preview Feb 4, 2026 6:00pm

@claude
Copy link
Contributor

claude bot commented Feb 4, 2026

Summary

This PR adds a simple JavaScript snippet to automatically collapse the Django admin sidebar by default on two recap pages: grants-recap.html and proposals-recap.html. The change improves the user experience by providing more horizontal space for viewing review data on these pages.

Review Feedback

Architecture & Design ✅

The implementation is straightforward and appropriate for the use case:

  • Uses Django's existing localStorage mechanism (django.admin.navSidebarIsOpen) for consistency with the admin interface
  • Properly placed in the extrahead block to execute before the page renders
  • No architectural concerns for this small UI enhancement

Testing & Coverage ⚠️

Missing test coverage:

  • No tests verify the localStorage behavior
  • Consider adding a browser/integration test to ensure:
    • The sidebar starts collapsed on these pages
    • Users can still manually expand/collapse the sidebar
    • The preference persists correctly when navigating away and back

This is a minor UI enhancement, so lack of tests is not critical, but it would be good practice to add basic coverage.

Error Handling ✅

The implementation is defensive:

  • Checks if the value is already 'false' before setting it to avoid unnecessary localStorage writes
  • Uses a simple conditional that won't throw errors
  • No error handling needed for this simple localStorage operation

Performance ✅

  • Minimal performance impact (single localStorage operation)
  • Executes in the <head> before page render to avoid visual flicker
  • No concerns here

Additional Observations

Potential issue - Logic flaw:
The condition localStorage.getItem('django.admin.navSidebarIsOpen') !== 'false' means the code will ONLY set the value to 'false' if it's NOT already 'false'. However, this means:

  • First visit: localStorage is null → sets to 'false' ✅
  • User manually opens sidebar: localStorage is 'true' → sets to 'false' ✅
  • User navigates away and returns: localStorage is 'false' → doesn't run ✅

Actually, on second thought, this logic is correct! It respects user preference if they've already closed the sidebar, while ensuring it starts collapsed for new visitors or those who have it open.

Documentation:
The PR description mentions "hopefully it's not a big deal to have the sidebar closed by default after opening the recap pages" - this suggests some uncertainty about the approach. Consider:

  • Does this affect other admin pages after users visit these recap pages? (No, because it only sets the value if not already 'false')
  • Should this be a per-page preference instead of global? The current implementation uses Django's global sidebar preference

Minor code style:

  • The JavaScript is consistent with the existing inline JavaScript in these templates
  • Could potentially be extracted to a shared JavaScript file, but given these are the only two pages with this requirement, inline is acceptable

Recommendation

Approve with minor suggestion: The implementation works correctly. Consider adding a comment explaining why we set the sidebar to collapsed on these specific pages (e.g., "These recap pages benefit from maximum horizontal space for viewing review data").

Optional follow-up: If you want per-page sidebar state (so that collapsing on recap pages doesn't affect other admin pages), you'd need to use a different localStorage key like django.admin.recapSidebarIsOpen instead.

@patrick91 patrick91 merged commit ed679d2 into main Feb 4, 2026
8 checks passed
@patrick91 patrick91 deleted the hide-sidebar-in-recap branch February 4, 2026 18:12
@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.16%. Comparing base (1439357) to head (518f4ef).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4556   +/-   ##
=======================================
  Coverage   94.16%   94.16%           
=======================================
  Files         353      353           
  Lines       10346    10346           
  Branches      761      761           
=======================================
  Hits         9742     9742           
  Misses        502      502           
  Partials      102      102
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@patrick91