Skip to content
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

Fixed #32443 -- Removed "shifted" CSS class when admin's sidebar is disabled. #13986

Merged

Conversation

theSinner
Copy link
Contributor

Hi

I faced an issue in disabling the nav-sidebar. When I disable the nav-sidebar using admin.site.enable_nav_sidebar = False, the shifted class is still remaining, because this class is hard-coded in the main tag. I remove the hard-coded class and add the shifted class whenever we have the side-navbar element on the page.

@theSinner
Copy link
Contributor Author

@felixxm
I don't know the proper way to ask for a review in this repo, So I use the manual way:D
Can you please review this PR and tell me your opinion?

@ngnpope
Copy link
Member

ngnpope commented Feb 11, 2021

Hi @theSinner. I see you're new here, so I'll point you to the contributing guidelines.

It looks like you're trying to fix a bug in the admin sidebar that was added in Django 3.1. The first thing is to create a ticket in the issue tracker clearly explaining the bug and how to reproduce it. In this case I would suggest attaching some screenshots as that will help a lot.

@theSinner
Copy link
Contributor Author

Hi @pope1ni
Thanks. I wanted to add a ticket but I read in the contributing document which there is no need to create a ticket for small bugs.
I created a ticket and attach some screenshots to describe the problem better.
https://code.djangoproject.com/ticket/32443

@felixxm felixxm changed the title Remove shifted class from main when nav-sidebar is disabled. Fixed #32443 -- Removed "shifted" CSS class when admin's sidebar is disabled. Feb 15, 2021
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theSinner Thanks for this patch 👍 Can we add a small assertion? e.g.

diff --git a/tests/admin_views/test_nav_sidebar.py b/tests/admin_views/test_nav_sidebar.py
index b3d4955b19..0f2733f0ee 100644
--- a/tests/admin_views/test_nav_sidebar.py
+++ b/tests/admin_views/test_nav_sidebar.py
@@ -43,6 +43,7 @@ class AdminSidebarTests(TestCase):
 
     def test_sidebar_disabled(self):
         response = self.client.get(reverse('test_without_sidebar:index'))
+        self.assertContains(response, '<div class="main" id="main">')
         self.assertNotContains(response, '<nav class="sticky" id="nav-sidebar">')
 
     def test_sidebar_unauthenticated(self):

django/contrib/admin/static/admin/js/nav_sidebar.js Outdated Show resolved Hide resolved
…lways false. Add assertion in the test_sidebar_not_on_index to make sure the main tag doesnt have the shifted class
@felixxm
Copy link
Member

felixxm commented Feb 18, 2021

buildbot, test on selenium.

@theSinner
Copy link
Contributor Author

buildbot, test on selenium.
@felixxm

I was reading the test_select_multiple test to find out the problem, I didn't find anything related to my changes.
I came here to ask you for help but I saw all the tests have been successful. Thanks by the way if you fixed that.

@felixxm felixxm merged commit 1710cdb into django:master Feb 18, 2021
@felixxm
Copy link
Member

felixxm commented Feb 18, 2021

@theSinner Thanks 👍 Welcome aboard ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants