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

Update the list of watchers and stargazers when clicking watch/unwatch or star/unstar #32570

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

yardenshoham
Copy link
Member

We make sure the user cards are updated

I also removed ctx.Data["PageIsWatchers"] = true and ctx.Data["PageIsStargazers"] = true as they are not used anywhere.

Before

before

After

after

…h or star/unstar

We make sure the user cards are updated

Signed-off-by: Yarden Shoham <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 19, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 19, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Nov 19, 2024
@lunny lunny added type/bug backport/v1.22 This PR should be backported to Gitea 1.22 labels Nov 19, 2024
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I would still strongly prefer to reload the whole page, instead of adding more special code (and the new code has no test code / is hard to test)

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should mix HTMX terminology directly into Gitea:
It looks like the /cards routes always (re-)render a list of users.
To me as a reader of the code, the /cards prefix reads strange.
I can understand it in this PR as it is HTMX focused, but the moment it is merged you won't have that context anymore.
So, perhaps we can find another name for it.

But functionality-wise LGTM.

routers/web/repo/view.go Show resolved Hide resolved
@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 Nov 20, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Nov 20, 2024
@silverwind
Copy link
Member

silverwind commented Nov 20, 2024

What happens to any potential JS event handlers in the swapped HTML when HTMX does the swap? How do we make sure that event handlers that need to be added to new cards are bound correctly after the swap? Likely there are currently no such event handlers, but it is something that must be considered when doing such swaps.

@yardenshoham
Copy link
Member Author

This is true for any HTMX swap. This pull request is not the first one. There are no event handlers attached to the cards so we should be ok.

hx-trigger="refreshCards from:body"
hx-indicator=".no-loading-indicator"
hx-swap="innerHTML"
hx-get="{{$.Link}}/cards"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it could use hx-select to partially reloading, then no need to add these /cards endpoints.

Copy link
Member Author

Choose a reason for hiding this comment

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

i will try it

Copy link
Contributor

Choose a reason for hiding this comment

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

Just pushed 🤣

Managed to get it work in da86679, only a few changed lines now

Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing, it should use CurrentURL but not Link, because there could be "page" query parameters.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 21, 2024
@github-actions github-actions bot removed the modifies/api This PR adds API routes or modifies them label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking Star/Unstar, Watch/Unwatch buttons on star list or watch list page result in inconsistent result.
6 participants