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

KTOR-7584 HTMX extension for Ktor #4553

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

KTOR-7584 HTMX extension for Ktor #4553

wants to merge 1 commit into from

Conversation

bjhham
Copy link
Contributor

@bjhham bjhham commented Dec 16, 2024

Subsystem
Server, New plugin

Motivation
KTOR-7584 First-class HTMX support

Solution
This extends the routing and HTML builder DSL with some handy HTMX helpers. I tried to ere on the conservative side here by not including everything and adding an experimental flag so we can change things later after testing. We don't have the HTML fragments extension yet in the HTML DSL, but you can still do everything you need using valid HTML and attributes.

@bjhham bjhham requested review from osipxd and Mr3zee December 16, 2024 13:34
Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Finally, it is here :)

Comment on lines +20 to +22
header(HxRequestHeaders.Request, "true") {
configuration()
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably this header should be applied to the route passed in HxRoute constructor:

public value class HxRoute(route: Route) : Route by route.header(HxRequestHeaders.Request, "true") {

@ExperimentalHtmxApi
@KtorDsl
@JvmInline
public value class HXRoute(private val route: Route) : Route by route {
Copy link
Member

Choose a reason for hiding this comment

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

Minor

Suggested change
public value class HXRoute(private val route: Route) : Route by route {
public value class HxRoute(private val route: Route) : Route by route {

client.get("htmx") {
headers[HxRequestHeaders.Request] = "true"
headers[HxRequestHeaders.Trigger] = "#button"
}.bodyAsText().trim()
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add negative tests?


@ExperimentalHtmxApi
@HtmlTagMarker
public class HxAttributes(override val map: DelegatingMap) : StringMapDelegate {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in a shared module?


public object HxAttributeKeys {
/**
* issues a GET to the specified URL
Copy link
Member

Choose a reason for hiding this comment

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

Here and in some other files KDocs are starting with a lowercase letter. Could you fix it?
I also think that we could add a link to the official docs to the KDoc of this class: https://htmx.org/reference/#attributes-additional

public var location: String? by HxResponseHeaders.Location
public var pushUrl: String? by HxResponseHeaders.PushUrl
public var redirect: String? by HxResponseHeaders.Redirect
public var refresh: String? by HxResponseHeaders.Refresh // TODO boolean
Copy link
Member

Choose a reason for hiding this comment

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

Should we make it boolean now? Or ExperimentalHtmxApi allows breaking changes? 😅

Comment on lines +34 to +39
public val on: On
get() = On(map)

public fun on(event: String, script: String) {
map["hx-on:$event"] = script
}
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 remember what we decided here. Keep both options of syntax?

on["event] = "script"
on("event", "script")

public var pushUrl: String? by HxAttributeKeys.PushUrl
public var select: String? by HxAttributeKeys.Select
public var selectOob: String? by HxAttributeKeys.SelectOob
public var swap: String? by HxAttributeKeys.Swap
Copy link
Member

Choose a reason for hiding this comment

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

We've had type-safe implementations for some of these fields. Will it be part of some future PR?

@osipxd osipxd force-pushed the 3.1.0-eap branch 2 times, most recently from 7c22c8a to f555d1a Compare December 19, 2024 10:14
@osipxd osipxd force-pushed the 3.1.0-eap branch 5 times, most recently from 2c3b4d8 to 4a59355 Compare January 9, 2025 14:59
Base automatically changed from 3.1.0-eap to main January 9, 2025 15:50
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.

2 participants