wimdeblauwe / htmx-spring-boot

Spring Boot and Thymeleaf helpers for working with htmx
Apache License 2.0
436 stars 41 forks source link

Add annotation support and HtmxResponse methods for all existing htmx response headers #67

Closed xhaggi closed 9 months ago

xhaggi commented 11 months ago

This adds annotation support and HtmxResponse methods for all currently existing htmx Response Headers. It also introduces a builder in HtmxResponse.

Please check the respective commits.

wimdeblauwe commented 11 months ago

impressive PR, thank you!

xhaggi commented 11 months ago

I'm done, probably a little more 😄. I added an additional commit Add support for specifying modifiers in HX-Reswap which changes the implementation to support all modifiers via the HtmxReswap class. I updated the commit Add annotation support for HX-Reswap to support all modifiers in the annotation as well.

@HxReswap(value = HxSwapType.INNER_HTML, swap = 300)

vs.

return HtmxResponse.builder()
  .reswap(HtmxReswap.innerHtml().swap(Duration.ofMillis(300)))
  .build();

I also rewrote the HX-Trigger implementation in HtmxResponse to support complex event details via Map. To simplify things a bit, I introduced a new class HtmxTrigger and dedicated builder methods for each case.

return HtmxResponse.builder()
  .trigger("event1")
  .trigger("event2", Map.of("var", "value"))
  .triggerAfterSettle("event3")
  .triggerAfterSettle("event4", Map.of("var", "value"))
  .triggerAfterSwap("event5")
  .triggerAfterSwap("event6", Map.of("var", "value"))
  .build();

It would really make sense to start a fresh review to not miss anything.

xhaggi commented 11 months ago

I have added HxSwapType.INNER_HTML as a default value in the HxReswap annotation, so there is no need to redefine this if you just want to customize a modifier e.g. swap.

xhaggi commented 11 months ago

I have added 2 more commits to update the README.md. I hope it's ok that I added the javadoc.io badge and link part of the README to javadoc.io.

xhaggi commented 10 months ago

@wimdeblauwe just wanted to ask if anything else is missing? Or is there something else to discuss besides the open comments?

wimdeblauwe commented 10 months ago

Thank you for your work so far, but I wonder if we can split up the PR into multiple PR's? Because it is really big as it is now and I am loosing a bit the full picture like this. If possible, I would like to have separate PR's for:

  1. Adding the javadoc.io badge and linking to specific pages from the readme file.
  2. Reworking HtmxResponse to use the builder pattern (Not sure if we should do this, so having this in a separate PR makes it easier to ask for other people's opinion on this).
  3. HtmxRequest renames of methods to remove the get/set prefix. If we do this, we should keep the old methods and deprecate them first to avoid people getting compilation errors on upgrading (I would like to avoid bumping the major version for this, again not sure if this brings much value to the project)
  4. Add Javadoc to existing code. This can be a straighforward addition and a PR I can merge without any problem of having breaking changes.
  5. Additional annotations. Ideally, we could have a separate PR per annotation, but I understand that would be a lot of work and might be a bit easier to group those together in 1 PR.

If you see other ways to break up this big PR, I am also open for discussion on this.

xhaggi commented 10 months ago

I don't want to spend too much time splitting it up, since there is no real advantage and many of the commits rely on each other. I definitely understand your concerns about introducing the builder pattern for HtmxResponse. My intention here was to make the implementation more like existing classes such as ResponseEntity, which also use a builder pattern (without with prefix). Therefore, I would suggest to keep the old implementation, mark it as deprecated and refer to the new builder pattern. The changes to HtmxRequest should be fairly safe, since HtmxRequest is not or should not be built by yourself. We could keep the changes or we could mark the methods as deprecated, as you like?

In my opinion, I would then split up the following commits into separate PRs.

  1. Wrong response header HX-Push instead of HX-Push-Url used
  2. Add missing Javadoc + Add javadoc.io badge to README

Let me know what we want to do.

wimdeblauwe commented 10 months ago

@checketts @odrotbohm I would like to hear what you think on this.

checketts commented 10 months ago

I second the idea of splitting these changes into several smaller ones. If they truly need to build on each other, perhaps creating the PRs one at a time will be more useful.

If the original author isn't able to split them up, a separate issue for each effort, discussing the change would be another option, but won't solve the ability to merge in changes as things get resolved.

I don't mean to make extra work for @xhaggi just trying to approach this in a way that we can make progress bit by bit.

xhaggi commented 10 months ago

Ok, let's start with the first round of splitting things up. There are now 4 new PR's to review. After these are merged, I will create another one that introduces the builder pattern into HtmxResponse which also keeps backward compatibility.

checketts commented 10 months ago

Thanks @xhaggi that made it much easier to review. Thanks for your contributions!

wimdeblauwe commented 9 months ago

@xhaggi I suppose I can close this PR now as we have the others now?

xhaggi commented 9 months ago

If that's OK with you, I'd like to leave this open until all the missing commits are in separate PRs.

xhaggi commented 9 months ago

BTW you can convert the pull request to a draft.

xhaggi commented 9 months ago

@wimdeblauwe, we have nearly everything merged, expect the annotation support, which is something for 4.0. IMO you can now close this PR. And after the 2 open pull requests have been merged, we could finally release version 3.1.

wimdeblauwe commented 9 months ago

@wimdeblauwe, we have nearly everything merged, expect the annotation support, which is something for 4.0. IMO you can now close this PR. And after the 2 open pull requests have been merged, we could finally release version 3.1.

ok. I will close this PR. You have done amazing work on this version, thank you very, very much!

xhaggi commented 9 months ago

I'm glad if I can contribute to move things forward 😄