htmx icon indicating copy to clipboard operation
htmx copied to clipboard

preloaded places `htmx-request` class on `body`

Open Hades32 opened this issue 1 year ago • 13 comments

Thanks for this fantastic piece of software!

I found a minor issue. When triggering preloading (by hovering) of some element, some other elements on my page suddenly got a new style. I found this was because preload places the htmx-request class on body. Probably this is the culprit, as it doesn't set anything. (Or the code that interprets undefined as body of course) https://github.com/bigskysoftware/htmx/blob/d766c14aa6aead788519fdf037f8c6387106c460/src/ext/preload.js#L51-L53

IMO it shouldn't place the class at all, but if it must, then it should be on the element that triggered it

Hades32 avatar May 16 '23 15:05 Hades32

@benpate or is this intended behavior?

Hades32 avatar May 18 '23 20:05 Hades32

Hola @Hades32 -

This isn't exactly intended behavior, so much as the default way that htmx handles requests. In the code you're highlighting, we just call the standard htmx.ajax() function and allow it to do its natural thing. Therefore you should be able to control where this goes using the hx-indicator attribute -- but again, that's the default behavior and I haven't tested this specifically.

Does this help? If not, we can try to make some changes to the extension so that it "does the right thing" -- whatever that it :)

benpate avatar May 20 '23 23:05 benpate

Thanks for your response @benpate. I'm not sure if changing the indicator target would work, but IMHO this would still be wrong. E.g. I play an animation on a button when the request is fired and I want it to preload on hover. BUT of course I don't want the button animation to play when I hover. And with the current behavior (which seems to default target the body element) all my buttons animate when I hover one of them.

So, not sure how to do this implementation-wise, but in my opinion preloading should not set the htmx-request tag at all, as it's semantically incorrect. At least not by default, or at the very least not when using the mouseover trigger

Hades32 avatar May 21 '23 05:05 Hades32

Yes, you're right. It's not "correct" as in the right thing to do. It is something that we've inherited from the default behavior, and will be pretty tough to fix right now without duplicating the htmx.ajax() function.

I am hoping for some underlying structural changes in htmx 2.0 that could make this more realistic to fix. @1cg - this is my tickler/request/reminder for splitting up the ajax and swap features of htmx. How realistic is this possibility in 2.0?

benpate avatar May 22 '23 00:05 benpate

@benpate I also would be interested in splitting the ajax and swap. I extended the preload extension to not only prime the cache but to save and replay the request (https://www.reddit.com/r/htmx/comments/147xjo4/preload_extension_only_primes_the_browsers_cache/)

In the current form, I still had to make a request to ensure htmx did the normal things, but it would be super to avoid that.

checketts avatar Jun 12 '23 22:06 checketts

Hey @checketts - I answered you on Reddit. You're 100% correct. Thanks for keeping this issue visible. I'm looking forward to updates in htmx that will make preload (and a bunch of other stuff) a million times better.

benpate avatar Jun 13 '23 01:06 benpate

I believe I found the root cause of the class being placed on the body (see PR #1493)

Since no source element is provided in the htmx.ajax call it default to the document body.

checketts avatar Jun 15 '23 03:06 checketts

Great. But I'd still argue that preloading shouldn't place the request class anywhere, as it is just an implementation detail that shouldn't leak to user-visible state

Hades32 avatar Jun 15 '23 05:06 Hades32

I agree. Here I was able to find a solution:

During the preload we can provide our own hx-indicator. This works but has the problem of logging an error that '.preload-indicator returns no matches' if it isn't present in your application.

var previousIndicator = node.getAttribute("hx-indicator");
node.setAttribute("hx-indicator", ".preload-indicator")

htmx.ajax("GET", hxGet, {
    source: node,
    handler:function(elt, info) {
        done(info.xhr.responseText);
    }});
if(previousIndicator) {
    node.setAttribute("hx-indicator", previousIndicator)
} else {
    node.removeAttribute("hx-indicator")
}

@benpate @1cg I would like to propose that hx-indicator be able to support false or none as options to disable the behavior. Would a PR for that in htmx core be acceptable?

Also would adding support for a preload-indicator be a good idea for the extension? I would be happy to send both changes.

checketts avatar Jun 15 '23 16:06 checketts

hx-indicator should support none since that's a valid css selector that will return nothing (unless someone has a custom element called none, in which case they get what they deserve)

1cg avatar Jun 15 '23 16:06 1cg

@1cg None 'works' but causes a console error (see https://github.com/bigskysoftware/htmx/blob/b4a61c543b283eb2315a47708006783efb78f563/src/htmx.js#L678)

The selector "false" on hx-indicator returned no matches!

Looking at the code I see that hx-indicator="" works, and avoids the error. So no core change is needed.

checketts avatar Jun 15 '23 20:06 checketts

@Hades32 I submitted a pr that addresses this behavior. See #1511

checketts avatar Jun 22 '23 22:06 checketts

Awesome! Thank you for keeping working on this!

Hades32 avatar Jun 23 '23 05:06 Hades32