-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Attach handlers to djDebug instead of document #1702
Attach handlers to djDebug instead of document #1702
Conversation
Doesn't the debug element get dropped when one of these libraries replaces the content of |
Great observation. There is a tag in Turbo that extract elements and replace them in the page after document is replaced. So adding the following setting makes things work:
I tried a number of other approaches to work around this, but this change was the straightforward & reliable way to ensure the proper behavior. |
Let's step back.
Can you clarify these issues? I know what this change is doing, but the PR is lacking information on why we need it. |
ScreenFlow.mp4Best described with an example. |
I appreciate your efforts, but videos are not helpful when looking at the project historically. We can't search what's said in the video versus the text in comments / the issue itself. |
It may be more helpful to create an additional test page within the example app for turbo. I'm not sure why our current implementation works for htmx, but not turbo. |
I can add that. It looks like htmx may replace individual elements, and while Turbo does the same for form submissions and websocket data, it replaces the entire document element for a page transition. It will first extract elements with the attribute |
This unfortunately breaks the integration with htmx. I think we may need to rework our solution entirely to better support these types of libraries. |
Verified the htmx integration was less than perfect even before - with the handle click events also getting lost when closing and reopening the toolbar. I think I've got it solved with my latest changes to the branch. Both expect users of html-swapping libraries to add to the @tim-schilling Thanks for pushing back on this to make sure it works for as many users as possible. |
@tim-schilling Could you give this a review this week? |
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.
I didn't try it out, just commenting here for now.
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.
Thank you @scuml for working through this. I like how it ended up. I did one last update to the docs to start linking to various parts more.
If you have time, can you rebase on main? Otherwise I'll try to get to it over the weekend. |
Good to go. Had to remove the |
Adds example page for turbo Add a few additional links for the tips on htmx and Turbo. Providing the links to the libraries' documentation may eventually go stale, but it should help the developer understand what's actually happening. Co-authored-by: Matthias Kestenholz <mk@feinheit.ch> Co-authored-by: Tim Schilling <schillingt@better-simple.com>
42d9815
to
00bc368
Compare
@scuml it can be a little tricky rebasing on main. You need to make sure your version of main is up to date with the upstream's (https://github.com/jazzband/django-debug-toolbar/) before doing the rebase (or merge in your case). I've gone ahead and squashed the commits in this PR and got it inline with main. Thank you for your work on this PR and advancing the toolbar! |
You're welcome. Glad to contribute. Thanks for leading as maintainer. |
Attach click handlers to the debug element itself instead of document body. This fixes issues when using libraries that replace the body element without reloading the page like Hotwire Turbo, htmx, and Unpoly.