Don't show loading indicators when refreshing the system status #30712

yardenshoham · 2024-04-26T07:30:18Z

When I added this the dividers didn't render the loading indicators but I guess some CSS has changed since then. Now we render the loading indicator on a hidden element so it won't appear.

Before

After

When I added this the dividers didn't render the loading indicators but I guess some CSS has changed since then. Now we render the loading indicators on hidden elements so they don't appear. Signed-off-by: Yarden Shoham <git@yardenshoham.com>

silverwind · 2024-04-26T08:02:20Z

Hmm,

The hx-indicator attribute allows you to specify the element that will have the htmx-request class added to it for the duration of the request

What's the point of .tw-hidden.htmx-request? Why not just remove hx-indicator if we don't want a indicator?

yardenshoham · 2024-04-26T09:03:24Z

We want the indicator for other htmx-based requests such as subscribe and watch

silverwind · 2024-04-26T09:05:45Z

Yeah but can't we remove it from this one element?

yardenshoham · 2024-04-26T11:25:12Z

I didn't see any other option

templates/admin/dashboard.tmpl

silverwind · 2024-04-26T11:32:41Z

So the problem is htmx can not distinguish between initial and subsequent requests, e.g. you can't conditionally show hx-indicator?

wxiaoguang · 2024-04-26T16:15:16Z

templates/admin/dashboard.tmpl

@@ -76,7 +76,7 @@
 			{{ctx.Locale.Tr "admin.dashboard.system_status"}}
 		</h4>
 		{{/* TODO: make these stats work in multi-server deployments, likely needs per-server stats in DB */}}
-		<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".divider" class="ui attached table segment">
+		<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".tw-hidden" class="ui attached table segment">


Actually it could also use something like hx-indicator="no-indicator" , because no-indicator also matches nothing.

Or even as simple as hx-indicator="no", we would never use <no> tag in HTML code, so it also matches nothing.

But that prints an error in the console on every request

I see, then tw-hidden also doesn't seem good enough because tw-hidden might also not exist.

Ideally it could be like this:

Suggested change

<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".tw-hidden" class="ui attached table segment">

<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".hidden-loading-indicator" class="ui attached table segment">

Then put <div class="hidden-loading-indicator tw-hidden"></div> into "admin/system_status"

Then everything is clear and I think it reads better.

And more details, .tw-hidden can't be right. It just adds the indicator to all the tw-hidden elements on this page, which causes various side effects

Think about a case:

<div class="panel tw-hidden"></div> <div hx-get hx-indicator=".tw-hidden"></div>

The "panel" is not related to the hx-get.

When the loading-indicator is added to the "panel", and the request is still running, then the user shows the panel: surprise: they sees a "loading panel".

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

templates/admin/dashboard.tmpl

Co-authored-by: silverwind <me@silverwind.io>

silverwind

It's a hack, but I guess a barely acceptable one. Would prefer a solution in htmx to suppress indicator on specific elements.

wxiaoguang · 2024-04-27T11:59:32Z

Why revert? Here we can clearly see htmx is using querySelectorAll:

You forgot to change the related code.

silverwind · 2024-04-27T12:00:10Z

Oh, I see, we should remove this useless element.

silverwind · 2024-04-27T12:01:13Z

Will test it and then remove it.

wxiaoguang · 2024-04-27T12:01:17Z

Oh, I seem we should remove this.

I do not see any reason to block merging this change.

silverwind · 2024-04-27T12:04:41Z

Fixed and tested.

wxiaoguang · 2024-04-27T12:05:03Z

Fixed and tested.

NOOOOOOOOOOOOOOOO! See the discussion above.

#30712 (comment)

silverwind · 2024-04-27T12:09:44Z

Hmm it does seem htmx falls back to the element if nothing matches:

https://github.com/bigskysoftware/htmx/blob/b176784f31a7bc4b55e40cec368dff571c4fb991/src/htmx.js#L2456-L2459

So your fix is better.

wxiaoguang · 2024-04-27T12:11:45Z

Hmm it does seem htmx falls back to the element if nothing matches:

It doens't fallback, it only returns an empty array. But there will be a lot of error messages in user's console, it would confuse users and developers. The root problem is that htmx insists to find something to show the indicator. If htmx could change its behavior to avoiding showing the indicator, life will be easier.

silverwind · 2024-04-27T12:11:50Z

Anyways, I have to add a remark that I'm starting to despise htmx. It's clearly not a well thought-out framework.

silverwind · 2024-04-27T12:14:13Z

It doens't fallback, it only returns an empty array. But there will be a lot of error messages in user's console, it would confuse users and developers

Right, querySelectorAll returns empty NodeList, never returns null.

silverwind · 2024-04-27T12:20:31Z

Upstream issue: bigskysoftware/htmx#2515

* giteaofficial/main: Make Ctrl+Enter work for issue/comment edit (go-gitea#30720) Rename migration package name for 1.22-rc1 (go-gitea#30730) Issue card improvements (go-gitea#30687) Don't show loading indicators when refreshing the system status (go-gitea#30712) Add some tests to clarify the "must-change-password" behavior (go-gitea#30693) Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686) Update JS dependencies (go-gitea#30713) Improve diff stats bar (go-gitea#30669) Remove unused parameter for some functions in `services/mirror` (go-gitea#30724) Update misspell to 0.5.1 and add `misspellings.csv` (go-gitea#30573) Suppress browserslist warning in webpack target (go-gitea#30571) [skip ci] Updated translations via Crowdin Diff color enhancements, add line number background (go-gitea#30670)

* origin/main: Replace deprecated `math/rand` functions (go-gitea#30733) Make Ctrl+Enter work for issue/comment edit (go-gitea#30720) Rename migration package name for 1.22-rc1 (go-gitea#30730) Issue card improvements (go-gitea#30687) Don't show loading indicators when refreshing the system status (go-gitea#30712) Add some tests to clarify the "must-change-password" behavior (go-gitea#30693) Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686) Update JS dependencies (go-gitea#30713) Improve diff stats bar (go-gitea#30669) Remove unused parameter for some functions in `services/mirror` (go-gitea#30724) Update misspell to 0.5.1 and add `misspellings.csv` (go-gitea#30573) Suppress browserslist warning in webpack target (go-gitea#30571) [skip ci] Updated translations via Crowdin Diff color enhancements, add line number background (go-gitea#30670) feat(api): enhance Actions Secrets Management API for repository (go-gitea#30656) Fix code search input for different views (go-gitea#30678) Fix incorrect object id hash function (go-gitea#30708)

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2024

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 26, 2024

github-actions bot added the modifies/templates This PR modifies the template files label Apr 26, 2024

yardenshoham force-pushed the fix-htmx-hidden branch from 8694552 to 1f4ad26 Compare April 26, 2024 07:30

silverwind reviewed Apr 26, 2024

View reviewed changes

templates/admin/dashboard.tmpl Outdated Show resolved Hide resolved

wxiaoguang reviewed Apr 26, 2024

View reviewed changes

yardenshoham and others added 2 commits April 26, 2024 20:41

Update templates/admin/dashboard.tmpl

69da05d

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Add hidden div

ae8061b

wxiaoguang approved these changes Apr 26, 2024

View reviewed changes

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 Apr 26, 2024

silverwind reviewed Apr 26, 2024

View reviewed changes

templates/admin/dashboard.tmpl Outdated Show resolved Hide resolved

yardenshoham and others added 2 commits April 26, 2024 22:37

Update templates/admin/dashboard.tmpl

2a8de1e

Co-authored-by: silverwind <me@silverwind.io>

hidden -> no

9f76d74

silverwind approved these changes Apr 26, 2024

View reviewed changes

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 Apr 26, 2024

wxiaoguang enabled auto-merge (squash) April 27, 2024 11:20

Merge branch 'main' into fix-htmx-hidden

12b1301

wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024

wxiaoguang and others added 2 commits April 27, 2024 19:23

Merge branch 'main' into fix-htmx-hidden

9cccd81

Merge branch 'main' into fix-htmx-hidden

4ba0435

wxiaoguang added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug labels Apr 27, 2024

silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024

silverwind self-requested a review April 27, 2024 12:00

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Apr 27, 2024

remove useless element

591617d

silverwind approved these changes Apr 27, 2024

View reviewed changes

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 Apr 27, 2024

Update dashboard.tmpl

c92de87

silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024

Merge branch 'main' into fix-htmx-hidden

9a00d63

wxiaoguang merged commit 51c28d9 into go-gitea:main Apr 27, 2024
26 checks passed

wxiaoguang deleted the fix-htmx-hidden branch April 27, 2024 13:05

GiteaBot added this to the 1.23.0 milestone Apr 27, 2024

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024

wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't show loading indicators when refreshing the system status #30712

Don't show loading indicators when refreshing the system status #30712

yardenshoham commented Apr 26, 2024 •

edited

silverwind commented Apr 26, 2024 •

edited

yardenshoham commented Apr 26, 2024

silverwind commented Apr 26, 2024

yardenshoham commented Apr 26, 2024

silverwind commented Apr 26, 2024 •

edited

This comment was marked as outdated.

wxiaoguang Apr 26, 2024 •

edited

yardenshoham Apr 26, 2024

wxiaoguang Apr 26, 2024 •

edited

wxiaoguang Apr 26, 2024

silverwind left a comment

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024 •

edited

silverwind commented Apr 27, 2024

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024

wxiaoguang commented Apr 27, 2024 •

edited

silverwind commented Apr 27, 2024 •

edited

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024 •

edited

silverwind commented Apr 27, 2024

silverwind commented Apr 27, 2024

	<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".tw-hidden" class="ui attached table segment">
	<div hx-get="{{$.Link}}/system_status" hx-swap="morph:innerHTML" hx-trigger="every 5s" hx-indicator=".hidden-loading-indicator" class="ui attached table segment">

Don't show loading indicators when refreshing the system status #30712

Don't show loading indicators when refreshing the system status #30712

Conversation

yardenshoham commented Apr 26, 2024 • edited

Before

After

silverwind commented Apr 26, 2024 • edited

yardenshoham commented Apr 26, 2024

silverwind commented Apr 26, 2024

yardenshoham commented Apr 26, 2024

silverwind commented Apr 26, 2024 • edited

This comment was marked as outdated.

wxiaoguang Apr 26, 2024 • edited

Choose a reason for hiding this comment

yardenshoham Apr 26, 2024

Choose a reason for hiding this comment

wxiaoguang Apr 26, 2024 • edited

Choose a reason for hiding this comment

wxiaoguang Apr 26, 2024

Choose a reason for hiding this comment

silverwind left a comment

Choose a reason for hiding this comment

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024 • edited

silverwind commented Apr 27, 2024

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024

wxiaoguang commented Apr 27, 2024 • edited

silverwind commented Apr 27, 2024 • edited

wxiaoguang commented Apr 27, 2024

silverwind commented Apr 27, 2024 • edited

silverwind commented Apr 27, 2024

silverwind commented Apr 27, 2024

yardenshoham commented Apr 26, 2024 •

edited

silverwind commented Apr 26, 2024 •

edited

silverwind commented Apr 26, 2024 •

edited

wxiaoguang Apr 26, 2024 •

edited

wxiaoguang Apr 26, 2024 •

edited

silverwind commented Apr 27, 2024 •

edited

wxiaoguang commented Apr 27, 2024 •

edited

silverwind commented Apr 27, 2024 •

edited

silverwind commented Apr 27, 2024 •

edited