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

Don't do a full page load when clicking the subscribe button #28871

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

yardenshoham
Copy link
Member

  • Refactor the form around the subscribe button into its own template
  • Use htmx to perform the form submission
    • hx-boost="true" to prevent the default form submission behavior of a full page load
    • hx-sync="this:replace" to replace the current request (in case the button is clicked again before the response is returned)
    • hx-target="this" to replace the form tag with the new form tag
    • hx-push-url="false" to disable a change to the URL
    • hx-swap="show:no-scroll" to preserve the scroll position
  • Change the backend response to return a <form> tag instead of a redirect to the issue page
  • Include htmx.org in javascript imports

This change introduces htmx with the hope we could use it to make Gitea more reactive while keeping our "HTML rendered on the server" approach.

Before

before

After

after

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 20, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 20, 2024
@yardenshoham yardenshoham added the type/enhancement An improvement of existing functionality label Jan 20, 2024
- Refactor the form around the subscribe button into its own template
- Use htmx to perform the form submission
  - `hx-boost="true"` to prevent the default form submission behavior of a full page load
  - `hx-sync="this:replace"` to replace the current request (in case the button is clicked again before the response is returned)
  - `hx-target="this"` to replace the form tag with the new form tag
  - `hx-push-url="false"` to disable a change to the URL
  - `hx-swap="show:no-scroll"` to preserve the scroll position
- Change the backend response to return a `<form>` tag instead of a redirect to the issue page
- Include `htmx.org` in javascript imports

This change introduces htmx with the hope we could use it to make Gitea more reactive while keeping our "HTML rendered on the server" approach.

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@yardenshoham
Copy link
Member Author

From CI: ERROR in License: htmx.org@1.9.10 has disallowed license BSD 2-Clause. Can we add htmx or is it not allowed?

@delvh
Copy link
Member

delvh commented Jan 20, 2024

According to my quick search, BSD2 is compatible with MIT, so it should be possible.

@lunny
Copy link
Member

lunny commented Jan 20, 2024

I think we need enough disscuss before introducing htmx. Maybe a new issue with proposql is better.

@yardenshoham
Copy link
Member Author

I asked in discord and there was no negative response. We can discuss on this PR

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@yardenshoham yardenshoham marked this pull request as ready for review January 20, 2024 17:05
@lunny
Copy link
Member

lunny commented Jan 20, 2024

I asked in discord and there was no negative response. We can discuss on this PR

I think we have ever discussed htmx here #5937

@denyskon
Copy link
Member

I'm in favour of introducing HTMX, as in this case it allows for more elegant and functional UI without having to rebuild everything to a JS framework and without adding much overhead

@lunny
Copy link
Member

lunny commented Jan 20, 2024

So then we will mix the framework include vue3+jquery+htmx+go template on the same repository?

@yardenshoham
Copy link
Member Author

I think if I was using vue3 or jquery for this PR it would add more lines of code than using htmx

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 20, 2024
@6543
Copy link
Member

6543 commented Jan 20, 2024

HTMX looks promising!

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 20, 2024
@6543 6543 merged commit 14f6fcf into go-gitea:main Jan 20, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Jan 20, 2024
@yardenshoham yardenshoham deleted the subscribe-htmx branch January 20, 2024 19:45
@denyskon
Copy link
Member

Wasn't that maybe a bit too fast? If there are objections against using HTMX, we should discuss them.....

@6543
Copy link
Member

6543 commented Jan 20, 2024

Didn't you lgtm ?!?

@denyskon
Copy link
Member

I fully support this change, and it's also true that two lgtms are enough to merge a PR according to our guidelines. I'm just in favour of discussing all objections before merging :)

@6543
Copy link
Member

6543 commented Jan 20, 2024

Ah ok next time just reject and ask for it first 😆

Anyway atm I'm still a TOC member (till we get the vote results...) that did accept it ... so should be fine ;)

But i can bring it up to discust to the whole TOC if you'd like to.

@denyskon
Copy link
Member

As I said, I fully support this change. We finally need a thought-out decision-making process for such things, but we don't have one yet to I guess it it is fine 😆

@delvh
Copy link
Member

delvh commented Jan 20, 2024

You can also count in a tentative approval from me.
I cannot evaluate how good it is yet as I don't have enough experience with it.
I'd say we can simply try it out, and if we notice that it isn't for us, we can still remove it again.

@lunny
Copy link
Member

lunny commented Jan 21, 2024

@6543 You should not merge it too fast since we have more maintainers ecspcially @wxiaoguang and @silverwind haven't introduce their reviews. After all, this is a new framework/js library. We should consider it carefully.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 21, 2024

I do not think the problem has been clearly defined or resolved.

If htmx is introduced, now there are at least 3 frameworks in Gitea:

  • Vanilla JS
  • jQuery + Fomantic UI
  • Vue
  • htmx

Is there a clear roadmap for how to refactor legacy code and introduce new changes? For example:

  1. Will the jQuery code be there forever?
  2. How to implement a new UI feature, use Vanilla JS, or Fomantic UI, or htmx?

Then the question is: does htmx really help and will be widely used?

I have been following htmx for some time. My feeling is:

  • How does htmx handle error messages and network failure in Gitea? How does it handling the "loading" indicator?
  • The basic feature like "replace inner html" could be easily implement by a few lines in our framework, ex: we already have "link-action" and "form-fetch-action"
  • Some advanced features overlaps with existing mechanisms, eg: modal. Should new code use Fomantic Modal, Gitea's self modal or htmx's modal?
  • Some advanced features like "htmx script" is quite fragile and I don't know how to lint/test them. It would cause maintainability problems.

So, IMO the only useful part of htmx at the moment is the "replacing inner html by ajax", while it is quite simple and could (should) be implemented by existing "link-action" and "form-fetch-action". Other htmx features are unlikely useful in foreseeable future

@6543
Copy link
Member

6543 commented Jan 21, 2024

the consensis was that we do want to get rid of jQuery or has this changed?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 21, 2024

the consensis was that we do want to get rid of jQuery or has this changed?

I have asked many times: if new PRs come with jQuery, what should be done? Ask (force) the contributors to rewrite the code? Maintainer help to rewrite the code?

If few people would really like to spend time on rewriting legacy code and rewriting jQuery PRs code, how would the "get rid of jQuery" plan succeed?

More thoughts are here: #5937 (comment)


And jQuery is somewhat off-topic here. The key question is: "Then the question is: does htmx really help and will be widely used?"

@lunny
Copy link
Member

lunny commented Jan 21, 2024

I agree "Ask (force) the contributors to rewrite the code"

6543 added a commit to 6543-forks/gitea that referenced this pull request Jan 21, 2024
@6543 6543 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 21, 2024
@6543
Copy link
Member

6543 commented Jan 21, 2024

#28879

@6543
Copy link
Member

6543 commented Jan 21, 2024

well we should not talk at closed/merged issues/pulls so i created #28880

@6543 6543 mentioned this pull request Jan 21, 2024
@a1012112796
Copy link
Member

Hmm, I think it's necessary to do a full page load when clicking the subscribe button because It can help users to get the new comments on time.

@6543
Copy link
Member

6543 commented Jan 22, 2024

no that's not the point at all ?!?

if we want to see "live" updates on comments that's a totali different issue and we do have to have either a smal background loop checking if there is something new and then update the html (proper way) or reload the page (non proper way).

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 22, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Prevent anonymous container access if `RequireSignInView` is enabled (go-gitea#28877)
  Don't show new pr button when page is not compare pull (go-gitea#26431)
  Avoid duplicate JS error messages on UI (go-gitea#28873)
  Fix branch list bug which displayed default branch twice (go-gitea#28878)
  Revert adding htmx until we finaly decide to add it (go-gitea#28879)
  Don't do a full page load when clicking the follow button (go-gitea#28872)
  Don't do a full page load when clicking the subscribe button (go-gitea#28871)
  Fix incorrect PostgreSQL connection string for Unix sockets (go-gitea#28865)
  Run `npm audit fix` (go-gitea#28866)
  Fix migrate storage bug (go-gitea#28830)
  Set the `isPermaLink` attribute to `false` in the `guid` sub-element (go-gitea#28860)
  In administration documentation about environment variables, point to those for the Go runtime instead of Go compiler (go-gitea#28859)
  Move doctor package from modules to services (go-gitea#28856)
  Add support for sha256 repositories (go-gitea#23894)
  Fix incorrect action duration time when rerun the job before executed once (go-gitea#28364)
  Fix some RPM registry flaws (go-gitea#28782)
  tests: missing refs/ in bare repositories (go-gitea#28844)
  Fix archive creating LFS hooks and breaking pull requests (go-gitea#28848)
henrygoodman pushed a commit to henrygoodman/gitea that referenced this pull request Jan 31, 2024
…a#28871)

- Refactor the form around the subscribe button into its own template
- Use htmx to perform the form submission
- `hx-boost="true"` to prevent the default form submission behavior of a
full page load
- `hx-sync="this:replace"` to replace the current request (in case the
button is clicked again before the response is returned)
  - `hx-target="this"` to replace the form tag with the new form tag
  - `hx-push-url="false"` to disable a change to the URL
  - `hx-swap="show:no-scroll"` to preserve the scroll position
- Change the backend response to return a `<form>` tag instead of a
redirect to the issue page
- Include `htmx.org` in javascript imports

This change introduces htmx with the hope we could use it to make Gitea
more reactive while keeping our "HTML rendered on the server" approach.

# Before


![before](https://github.com/go-gitea/gitea/assets/20454870/4ec3e81e-4dbf-4338-9968-b0655c276d4c)

# After


![after](https://github.com/go-gitea/gitea/assets/20454870/8c8841af-9bfe-40b2-b1cd-cd1f3c90ba4d)

---------

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
…a#28871)

- Refactor the form around the subscribe button into its own template
- Use htmx to perform the form submission
- `hx-boost="true"` to prevent the default form submission behavior of a
full page load
- `hx-sync="this:replace"` to replace the current request (in case the
button is clicked again before the response is returned)
  - `hx-target="this"` to replace the form tag with the new form tag
  - `hx-push-url="false"` to disable a change to the URL
  - `hx-swap="show:no-scroll"` to preserve the scroll position
- Change the backend response to return a `<form>` tag instead of a
redirect to the issue page
- Include `htmx.org` in javascript imports

This change introduces htmx with the hope we could use it to make Gitea
more reactive while keeping our "HTML rendered on the server" approach.

# Before


![before](https://github.com/go-gitea/gitea/assets/20454870/4ec3e81e-4dbf-4338-9968-b0655c276d4c)

# After


![after](https://github.com/go-gitea/gitea/assets/20454870/8c8841af-9bfe-40b2-b1cd-cd1f3c90ba4d)

---------

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/internal size/M Denotes a PR that changes 30-99 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants