Add loading status to submit buttons #10459

jolheiser · 2020-02-25T18:11:38Z

Possible fix for #6859 and similar.

This is a "one size fits all solution" and so I'm not sure it's the best approach, open to criticism.

Perhaps it would be better to add this mechanism to individual buttons?

Signed-off-by: jolheiser <john.olheiser@gmail.com>

silverwind · 2020-02-25T18:36:34Z

Adding disabled will cause unwanted style changes. I would just add loading and do button.loading { pointer-events: none} which should prevent clicks.

Also, wtf is up with fomantic's CSS. The matching selector for a loading button is:

.ui.loading.loading.loading.loading.loading.loading.button

silverwind · 2020-02-25T18:38:34Z

Also, setTimeout is a very ugly hack. I think we need a more general approach to set loading states and unset them when requests finish (Thought I guess many buttons will cause page reloads where unset never happens).

I'm thinking something like below but it'd be a major refactor

button.addEventListener('click', async ({target}) => {
  target.classList.add('loading');
  await doRequest();
  target.classList.remove('loading');
});

jolheiser · 2020-02-25T18:40:24Z

setTimeout is used because if there is a form with required inputs that don't submit, it would otherwise "load" indefinitely.

I agree it's not ideal, but I wasn't sure how else to temporarily disable a button in the event the submit doesn't actually fire.

jolheiser · 2020-02-25T18:42:10Z

Seems that button.loading { pointer-events: none} still allowed a few clicks through because of the aforementioned .ui.loading.loading.loading.loading.loading.loading.button setting pointer-events: auto;

EDIT: Adding !important works because of course it does. Is this still preferable to the disabled class?

Signed-off-by: jolheiser <john.olheiser@gmail.com>

silverwind · 2020-02-25T19:29:07Z

setTimeout is used because if there is a form with required inputs that don't submit

Not sure how the form validation works but I guess there must be some way to query if a form is valid (there should be existing code as it's not possible to create a new repo with empty name).

Is this still preferable to the disabled class?

I don't like pointer-events much either, best would be a check for the existance of the loading class:

$('body').on('click', 'form.ui.form button.ui.green.button').click(function (e) {
  if (e.currentTarget.classList.contains('loading')) return e.preventDefault();
  // check validity of form somehow
  if (!valid) return e.preventDefault();
  e.currentTarget.classList.add('loading');
});

This assumes every form is submitted synchronously and navigation happens on submit. I'm pretty certain there are some ajax forms that would need to remove the loading class on completion/error.

It uses a delegated listener so it would work for forms inserted via JS too.

Note that button selector is probably too broad. There're "Cancel" buttons like on "New Repo" that probably don't need a loading state. Ideally all our submit buttons would have type=submit to detect them but it seems not the case.

silverwind · 2020-02-25T19:39:44Z

Maybe it'd be possible to hook into the <form> submit event to set the loading states on the submit button. That should theoretically only trigger on a valid form.

sapk · 2020-03-02T01:14:25Z

Maybe we could exclude failed form with the CSS selector :invalid and the sibling ~ match ?

zeripath · 2020-03-02T21:24:30Z

So those .loading.loading.loading.... classes make me wonder if an alternative to !important is to use button.loading.loading.loading.loading.loading.loading.loading.loading.loading?

jolheiser · 2020-03-02T21:34:36Z

So those .loading.loading.loading.... classes make me wonder if an alternative to !important is to use button.loading.loading.loading.loading.loading.loading.loading.loading.loading?

Probably, but I think I'd rather use !important than compare the length of .loading selectors. 😂

I think I might look into hooking the submit event.
If that doesn't work, perhaps for the short-term I can just add the "loading" status to specific forms, since this attaches to all of them when most probably don't really need it.

codecov-io · 2020-03-02T22:01:46Z

Codecov Report

Merging #10459 into master will increase coverage by 0.02%.
The diff coverage is 46.29%.

@@            Coverage Diff             @@
##           master   #10459      +/-   ##
==========================================
+ Coverage    43.7%   43.73%   +0.02%     
==========================================
  Files         586      585       -1     
  Lines       81411    82014     +603     
==========================================
+ Hits        35582    35866     +284     
- Misses      41428    41707     +279     
- Partials     4401     4441      +40

Impacted Files	Coverage Δ
routers/private/hook.go	`36.6% <ø> (+0.17%)`	⬆️
routers/private/internal.go	`82.6% <ø> (+51.96%)`	⬆️
modules/setting/setting.go	`44.07% <ø> (ø)`	⬆️
routers/api/v1/repo/pull.go	`34.63% <ø> (-0.19%)`	⬇️
modules/base/tool.go	`65.02% <ø> (+2.07%)`	⬆️
models/issue_label.go	`63.04% <ø> (-0.75%)`	⬇️
modules/templates/helper.go	`42.19% <ø> (+1.46%)`	⬆️
models/update.go	`8.95% <ø> (+1.16%)`	⬆️
services/pull/patch.go	`61.93% <ø> (-0.96%)`	⬇️
routers/api/v1/utils/utils.go	`76% <ø> (+12.66%)`	⬆️
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eb27a2...a32032c. Read the comment docs.

silverwind · 2020-03-02T23:30:32Z

Form submit event is probably also no good because you don't know which button was pressed (for example issue comment is two buttons).

I think current approach is almost fine but maybe you can try the "is valid" API to check validity before setting loading state. The timed reset is probably unavoidable for sync form submissions. For async ones, code should be able to set it.

stale · 2020-05-02T00:24:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

lunny · 2020-09-02T12:27:17Z

web_src/js/index.js

+    $this.addClass('loading');
+    setTimeout(() => {
+      $this.removeClass('loading');
+    }, 1000 * 5); // 5 seconds


We should enable the button only after received the HTTP response.

I believe this only applies to "real" forms, in which case receiving a response means the page is reloading I think?

This timeout is a fallback in case it fails so the button isn't stuck perpetually loading.

silverwind · 2020-09-07T17:35:21Z

Quoting myself again in case it was missed:

I think current approach is almost fine but maybe you can try the "is valid" API to check validity before setting loading state

So what this needs is at least:

Don't set loading state when form is invalid (e.g. probably search for :invalid fields or whatever the HTML5 API for that is. Fomantic API should actually be last-resort). Or maybe just cancel out the loading state a few 100ms later if fomantic prevents submission, but it's certainly not ideal to set loading state if the validation is client-side.
Dig through the JS code for any async submitted form that don't trigger a page reload and manueally handle loading state for them. I guess you could use classes form-sync or form-async to distinguish them.

jolheiser · 2020-09-08T21:27:47Z

I will try to look at this again soon. Thanks for the tips @silverwind

jolheiser · 2020-10-01T19:10:10Z

I had a thought about this the other day, perhaps rather than mess with disabling (and possibly re-enabling), would it make more sense to add a debounce to the buttons instead?

silverwind · 2020-10-01T19:21:53Z

The common debounce functions would still emit a second event at the end of the debounce duration, unless you want to delay the initial event but I don't think it's a good idea to introduce artifical latency.

zeripath · 2020-10-01T20:27:41Z

debounce would be better suited to our a label setting and notifications pages. I think a throttle/disable would be a better fit for our buttons

wxiaoguang · 2022-05-07T14:55:32Z

A similar PR has been merged:

Prevent double click new issue/pull/comment button #16157

lunny · 2022-05-07T16:45:18Z

conflicted with #16157, so let's close this one.

Add loading status to submit buttons

9e799ec

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser added the topic/ui Change the appearance of the Gitea UI label Feb 25, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2020

!important

6eb27a2

Signed-off-by: jolheiser <john.olheiser@gmail.com>

lafriks added this to the 1.12.0 milestone Feb 26, 2020

Merge branch 'master' into loading-button

a32032c

stale bot added the issue/stale label May 2, 2020

lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label May 2, 2020

stale bot removed the issue/stale label May 2, 2020

lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020

lunny requested changes Sep 4, 2020

View reviewed changes

lunny modified the milestones: 1.13.0, 1.14.0 Sep 5, 2020

lunny modified the milestones: 1.14.0, 1.15.0 Jan 27, 2021

techknowlogick modified the milestones: 1.15.0, 1.16.0 Jun 15, 2021

lunny mentioned this pull request Jun 16, 2021

Prevent double click new issue/pull/comment button #16157

Merged

lunny removed this from the 1.16.0 milestone Nov 9, 2021

wxiaoguang mentioned this pull request Dec 8, 2021

Click buttons quickly in some forms will submit repeated/duplicated requests #17111

Closed

6543 closed this May 7, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add loading status to submit buttons #10459

Add loading status to submit buttons #10459

jolheiser commented Feb 25, 2020

silverwind commented Feb 25, 2020

silverwind commented Feb 25, 2020 •

edited

Loading

jolheiser commented Feb 25, 2020

jolheiser commented Feb 25, 2020 •

edited

Loading

silverwind commented Feb 25, 2020 •

edited

Loading

silverwind commented Feb 25, 2020

sapk commented Mar 2, 2020

zeripath commented Mar 2, 2020

jolheiser commented Mar 2, 2020

codecov-io commented Mar 2, 2020

silverwind commented Mar 2, 2020 •

edited

Loading

stale bot commented May 2, 2020

lunny Sep 2, 2020

jolheiser Sep 6, 2020

silverwind commented Sep 7, 2020 •

edited

Loading

jolheiser commented Sep 8, 2020

jolheiser commented Oct 1, 2020

silverwind commented Oct 1, 2020 •

edited

Loading

zeripath commented Oct 1, 2020

wxiaoguang commented May 7, 2022

lunny commented May 7, 2022

Add loading status to submit buttons #10459

Add loading status to submit buttons #10459

Conversation

jolheiser commented Feb 25, 2020

silverwind commented Feb 25, 2020

silverwind commented Feb 25, 2020 • edited Loading

jolheiser commented Feb 25, 2020

jolheiser commented Feb 25, 2020 • edited Loading

silverwind commented Feb 25, 2020 • edited Loading

silverwind commented Feb 25, 2020

sapk commented Mar 2, 2020

zeripath commented Mar 2, 2020

jolheiser commented Mar 2, 2020

codecov-io commented Mar 2, 2020

Codecov Report

silverwind commented Mar 2, 2020 • edited Loading

stale bot commented May 2, 2020

lunny Sep 2, 2020

Choose a reason for hiding this comment

jolheiser Sep 6, 2020

Choose a reason for hiding this comment

silverwind commented Sep 7, 2020 • edited Loading

jolheiser commented Sep 8, 2020

jolheiser commented Oct 1, 2020

silverwind commented Oct 1, 2020 • edited Loading

zeripath commented Oct 1, 2020

wxiaoguang commented May 7, 2022

lunny commented May 7, 2022

silverwind commented Feb 25, 2020 •

edited

Loading

jolheiser commented Feb 25, 2020 •

edited

Loading

silverwind commented Feb 25, 2020 •

edited

Loading

silverwind commented Mar 2, 2020 •

edited

Loading

silverwind commented Sep 7, 2020 •

edited

Loading

silverwind commented Oct 1, 2020 •

edited

Loading