django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #32934 -- Set admin sidebar height to be dynamic based on visible header

Open hrushikeshrv opened this issue 2 years ago • 3 comments

A pure CSS solution for ticket #32934 addressed in PR #14281 instead of the javascript approach in the previous PR.

This solution uses calc() to responsively set the height of the navigation sidebar to be 100vh minus whatever the height of the header is, so that it does not extend past the fold, making scrolling through it a little easier. It uses a media query at 1024px.

One edge case is when the title of the header has been replaced with some very long text, which increases the height of the header even further. In that case, we just revert back to the current behavior and the navbar extends past the fold. This edge case is expected to be very rare, since the amount of text required in the header is long.

This achieves the previous PR's expected result - Before: 115156636-05c0c980-a03a-11eb-8dfb-b12b6ae010b6

After: 115156639-08bbba00-a03a-11eb-8eb1-c5f7cd2a94f2

hrushikeshrv avatar Dec 24 '21 03:12 hrushikeshrv

Hello @hrushikeshrv! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

github-actions[bot] avatar Dec 24 '21 03:12 github-actions[bot]

@hrushikeshrv I can see a strange side-effect of this patch, the sidebar is cut off at the bottom on the page: image Django 4.1b1: image

felixxm avatar Jul 13 '22 07:07 felixxm

@felixxm yes, reducing the height of the navbar leads to it being cut-off when the user scrolls down (since the height is no longer 100vh). Unfortunately, I think this is a limitation of CSS and/or sticky positioning, and I don't think there is any pure CSS fix for this issue (not 100% sure about that, though, but around 99% sure). One workaround might be also making the header and the breadcrumbs sticky, for e.g. -

image

In my opinion, this "feels" better to scroll through, since the header only takes around 120px at most, and it also keeps the "VIEW SITE", "CHANGE PASSWORD", "LOGOUT" links, as well as the breadcrumbs visible even on scroll, and also doesn't cause the navbar to look cut-off when the user scrolls. We could do this with some media queries, but I think a better solution could also involve refactoring app_list.html and/or app_index.html a little.

hrushikeshrv avatar Jul 13 '22 14:07 hrushikeshrv

Hi @hrushikeshrv — thanks for the effort here. Sorry for the pause in follow-up our end: it's one of those tricky ones.

Looking at this again, I think the issue that @felixxm points out is telling: once the header is scrolled away, the gap at the bottom of the sidebar isn't good at all.

Thus, I think we have to say that this simple approach isn't acceptable.

...and I don't think there is any pure CSS fix for this issue

We could do this with some media queries, but I think a better solution could also involve refactoring app_list.html and/or app_index.html a little.

In that case I think perhaps this isn't worth fixing.

On main, as it currently stands, if I scroll the left-sidebar it pauses, and begins to scroll in sync with the main content, until the bottom is reached... — yes, OK, that's not 100% independent scrolling... but I think it's OK. That's how just browsers work, I might think. (I don't see it much different from https://github.com/django/django/pull/14281#issuecomment-947378429)

So we have a minor issue, which looks like a full-workaround requires a quite large fix. I'm just not seeing that as warranted. So I think we should mark the ticket as wontfix, pending as actual proposal for a simple fix that covers the issues mentioned here and on #14281. If you still fancy working on it, that's super: if you hit a good solution we can easily reopen.

Thanks again.

carltongibson avatar Aug 17 '22 10:08 carltongibson

I agree with the wontfix for this. The work involved for fixing it would be a lot for a minor issue. I'll keep it in the back of my mind in the meanwhile in case I think of something!

hrushikeshrv avatar Aug 18 '22 02:08 hrushikeshrv

Great. Thanks @hrushikeshrv 👍

carltongibson avatar Aug 18 '22 05:08 carltongibson