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

The djdt handle shouldn't be stuck at the top of the browser window initially #1853 #1871

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

VeldaKiara
Copy link
Member

Description

The issue was as much as the toolbar can be moved, it would be preferred if its position was on the upper top instead of the top.

Fixes # (issue)
The issue is mentioned here #1853

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@@ -220,7 +220,7 @@
background-color: #fff;
border: 1px solid #111;
border-bottom: 0;
top: 0;
top: 268px;
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using a relative position rather than an fixed value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would work too. Let me try it out and see the changes

Copy link
Member Author

Choose a reason for hiding this comment

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

or what if instead of changing the position, I adjust the y translation?
What do you think?

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

I tried it out and the handle was still stuck at the top. Maybe I did something wrong, but it seems to me that a better way would be to change the fallback in ensureHandleVisibility to not be 0 but something like 200:

https://github.com/jazzband/django-debug-toolbar/blob/77aa47a761da203c52e2328990123abb252d8dd5/debug_toolbar/static/debug_toolbar/js/toolbar.js#L225-L233

I like Tim's idea of using a relative value for the default but maybe it will be harder to implement the handle dragging since you'd have to convert pixels to relative values there as well.

@VeldaKiara
Copy link
Member Author

I tried it out and the handle was still stuck at the top. Maybe I did something wrong, but it seems to me that a better way would be to change the fallback in ensureHandleVisibility to not be 0 but something like 200:

https://github.com/jazzband/django-debug-toolbar/blob/77aa47a761da203c52e2328990123abb252d8dd5/debug_toolbar/static/debug_toolbar/js/toolbar.js#L225-L233

I like Tim's idea of using a relative value for the default but maybe it will be harder to implement the handle dragging since you'd have to convert pixels to relative values there as well.

I noticed that if I use position relative, I will have to make numerous changes however I decided to leave the button as is because it was always getting overridden by the fallback function. You and @tim-schilling can test it out now to see if it works now.

@matthiask matthiask merged commit 22df01c into django-commons:main Jan 25, 2024
26 checks passed
@matthiask
Copy link
Member

Thank you!

@VeldaKiara VeldaKiara self-assigned this Jan 26, 2024
@VeldaKiara
Copy link
Member Author

Happy to help.

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

Successfully merging this pull request may close these issues.

3 participants