-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Deprecate RedirectsPanel and update tests for deprecation warning #2223
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
base: main
Are you sure you want to change the base?
Deprecate RedirectsPanel and update tests for deprecation warning #2223
Conversation
tests/panels/test_redirects.py
Outdated
|
||
def test_deprecation_warning(self): | ||
"""Test that a deprecation warning is shown when RedirectsPanel is instantiated.""" | ||
from debug_toolbar.toolbar import DebugToolbar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a local import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No will change it
Originally I was thinking it would be a message on the template that gets rendered for the |
Should I include it in the template too? |
@JohananOppongAmoateng I'm good with this as is. I wanted to raise it to see what you thought. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I'd like another @django-commons/django-debug-toolbar-committers to approve of the idea (not necessarily the code) before I merge this in.
…-toolbar into deprecate-redirects-panel
Hello, I think a message in the template it's not a lot of work and another warning for people who don't read the warning in console. |
Mm that's true I will add a warning in the template. Should it be something like a toast? |
Yep only if the user is on the template ofc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new styles look pretty fixed with pixel units and static colors. This needs to be more flexible for both themes and people who adjust the zoom on the page. The classes also need to be prefixed with the toolbar id (see other styles). This prevents our styles from impacting application styles.
Description
Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. Your commit message should include
this information as well.
Fixes #2216
Checklist:
docs/changes.rst
.