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

Support rerendering the toolbar on HTMX boosted pages. #1686

Merged
merged 6 commits into from Oct 23, 2022

Conversation

tim-schilling
Copy link
Contributor

This reworks the JavaScript integration to put most event handlers on document.body. This means we'll have slightly slower performance, but it's easier to handle re-rendering the toolbar when the DOM has been replaced.

@tim-schilling
Copy link
Contributor Author

@knyghty can you verify that this resolves the issue on your project?

.. code-block:: javascript

if (typeof htmx !== "undefined") {
htmx.on("htmx:afterSettle", function(detail) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should check for the existence of djdt. People are likely to paste this in and use it on production.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know htmx that well but does it ever happen that window.htmx is undefined on such pages?

Maybe const h = window.htmx, d = window.djdt; if (h && d) { ... }? This is all a matter of taste and a very personal opinion (not as a DJDT maintainer!) of course but I prefer window.<var> to the somewhat magic global <var>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided against creating constants and went with window.htmx. I did realize that we can't check window.djdt there since it may not be loaded in yet. So I pushed that check into the event handler, then wrapped it all with {% if debug %} to better signal that this should only be used when the toolbar is installed.

};
window.djdt = {
show_toolbar: djdt.show_toolbar,
hide_toolbar: djdt.hide_toolbar,
init: djdt.init,
close: djdt.hide_one_level,
cookie: djdt.cookie,
get_debug_element: djdt.get_debug_element,
Copy link
Member

@matthiask matthiask Oct 21, 2022

Choose a reason for hiding this comment

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

I'm a bit sad that we're using a mix of snake_case and camelCase in this file; camelCase seems to be more idiomatic when writing javascript since almost all of the JS runtime itself uses camelCase (especially the DOM) but consistency is more important in the end. Sorry for the borderline bikeshedding.

The more important question is maybe if get_debug_element really has to be exposed here? Can't the method "just" be an internal helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It can be internal. I'll switch it to camelCase. And then have another commit changing everything else to camelCase so we can stay consistent moving forward.

@knyghty
Copy link
Member

knyghty commented Oct 22, 2022

@tim-schilling once I use the snippet from the example it works fine, yes 👍🏻

This reworks the JavaScript integration to put most event handlers on document.body.
This means we'll have slightly slower performance, but it's easier
to handle re-rendering the toolbar when the DOM has been replaced.
This leaves the public API with two snake case functions, show_toolbar and
hide_toolbar.
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.

Thanks!

@tim-schilling tim-schilling merged commit 7a8920c into jazzband:main Oct 23, 2022
@tim-schilling tim-schilling deleted the htmx-boosted-support branch October 23, 2022 13:28
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