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

Attach handlers to djDebug instead of document #1702

Merged
merged 1 commit into from Dec 3, 2022

Conversation

scuml
Copy link
Contributor

@scuml scuml commented Nov 9, 2022

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.

@tim-schilling
Copy link
Contributor

Doesn't the debug element get dropped when one of these libraries replaces the content of body?

@scuml
Copy link
Contributor Author

scuml commented Nov 9, 2022

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:

DEBUG_TOOLBAR_CONFIG = {
    "ROOT_TAG_EXTRA_ATTRS": "data-turbo-permanent"
}

I tried a number of other approaches to work around this, but this change was the straightforward & reliable way to ensure the proper behavior.

@tim-schilling
Copy link
Contributor

Let's step back.

This fixes issues when using libraries that replace the body element without reloading the page like Hotwire Turbo, htmx, and Unpoly.

Can you clarify these issues? I know what this change is doing, but the PR is lacking information on why we need it.

@scuml
Copy link
Contributor Author

scuml commented Nov 9, 2022

ScreenFlow.mp4

Best described with an example.

@tim-schilling
Copy link
Contributor

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.

@tim-schilling
Copy link
Contributor

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.

@scuml
Copy link
Contributor Author

scuml commented Nov 9, 2022

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 data-turbo-permanent, and reinsert them on the page. So if we use the setting I mentioned above, along with this change, it allows django-toolbar to operate as expected with this library.

@tim-schilling
Copy link
Contributor

This unfortunately breaks the integration with htmx. I think we may need to rework our solution entirely to better support these types of libraries.

@scuml
Copy link
Contributor Author

scuml commented Nov 15, 2022

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 ROOT_TAG_EXTRA_ATTRS setting. hx-preserve for htmx, and data-turbo-permanent for Turbo. These will retain the toolbar and handle across page loads. Custom javascript listeners are no longer needed and have been removed from the htmx example. Docs have been updated with this information as well.

@tim-schilling Thanks for pushing back on this to make sure it works for as many users as possible.

@scuml
Copy link
Contributor Author

scuml commented Nov 21, 2022

@tim-schilling Could you give this a review this week?

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 didn't try it out, just commenting here for now.

example/settings.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tim-schilling tim-schilling left a 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.

@tim-schilling
Copy link
Contributor

If you have time, can you rebase on main? Otherwise I'll try to get to it over the weekend.

@scuml
Copy link
Contributor Author

scuml commented Dec 2, 2022

Good to go. Had to remove the enchant install as it was causing CI to fail, however, it doesn't appear needed.

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>
@tim-schilling
Copy link
Contributor

@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!

@scuml
Copy link
Contributor Author

scuml commented Dec 3, 2022

You're welcome. Glad to contribute. Thanks for leading as maintainer.

@tim-schilling tim-schilling merged commit 00bc368 into jazzband:main Dec 3, 2022
@scuml scuml deleted the attach-handlers-to-djdebug branch December 3, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants