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

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

Merged
merged 12 commits into from
May 7, 2022

Conversation

a1012112796
Copy link
Member

when network is not good, these button maybe
double clicked, then more than one same issues
pulls or comments will be created. this pull
request will fix this bug.

Before:
Peek 2021-06-15 11-57

Now:
Peek 2021-06-15 11-56

when network is not good, these button maybe
double clicked, then more than one same issues
pulls or comments will be created. this pull
request will fix this bug.

Signed-off-by: a1012112796 <1012112796@qq.com>
@a1012112796 a1012112796 added the topic/ui Change the appearance of the Gitea UI label Jun 15, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 15, 2021
web_src/js/index.js Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
Co-authored-by: silverwind <me@silverwind.io>
@KN4CK3R
Copy link
Member

KN4CK3R commented Jun 15, 2021

Almost every button should become a once-button.

@silverwind
Copy link
Member

I would call it loading-button.

@a1012112796
Copy link
Member Author

Almost every button should become a once-button.

Sure, all button which used to post a form by default logic can use this way. Will check more latter.

@codecov-commenter

This comment was marked as outdated.

@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 Jun 16, 2021
@techknowlogick techknowlogick added this to the 1.15.0 milestone Jun 16, 2021
@lunny
Copy link
Member

lunny commented Jun 16, 2021

Since almost button clicked will refresh the page, I think it's suitable. But once we convert to an ajax request button. It should be changed. @jolheiser has a similar PR #10459, but I think this one is better.

@a1012112796
Copy link
Member Author

Since almost button clicked will refresh the page, I think it's suitable. But once we convert to an ajax request button. It should be changed. @jolheiser has a similar PR #10459, but I think this one is better.

Hmm, timeout logic is usefull, I will check more later.

@a1012112796 a1012112796 marked this pull request as draft June 16, 2021 02:56
@a1012112796 a1012112796 marked this pull request as ready for review June 16, 2021 04:51
@a1012112796 a1012112796 added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 16, 2021
@zeripath
Copy link
Contributor

CI failing

@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 Jun 21, 2021
@stale stale bot added the issue/stale label May 2, 2022
web_src/js/index.js Outdated Show resolved Hide resolved
@stale stale bot removed the issue/stale label May 2, 2022
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Does this also will prevent with interacting with button twice using keyboard (sapcebar, enter)?

@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 May 6, 2022
Comment on lines +146 to +153
// loading-button this logic used to prevent push one form more than one time
$(document).on('click', '.button.loading-button', function (e) {
const $btn = $(this);

if ($btn.hasClass('loading')) {
e.preventDefault();
return false;
}
Copy link
Contributor

@wxiaoguang wxiaoguang May 6, 2022

Choose a reason for hiding this comment

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

I think you need capture instead of bubble here. Otherwise the event handler on the button would be executed first (for example: ajax, IIRC some form buttons are doing ajax requests)

https://stackoverflow.com/questions/4616694/what-is-event-bubbling-and-capturing

Copy link
Contributor

@wxiaoguang wxiaoguang May 6, 2022

Choose a reason for hiding this comment

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

To be clear, what I mean is:

If we are trying to introduce a general method, should the all cases be considered together? Will this mechanism be used for ajax buttons in future?

  • If this mechanism is for traditional forms (non-ajax) only, these CSS names are too broad, it will be mis-used by ajax buttons in future.

  • If this mechanism is for both traditional forms and ajax requests, it could be fine tuned some more.


update: I would suggest to use form-once-submit as the CSS name for these form-only buttons.

In future, we can have ajax-once-request CSS class for ajax buttons (if necessary), or ajax buttons should handle the repeated clicks by themselves.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be generic for all use cases where technically possible and needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Either is fine to me:

  • Make the CSS name in this PR clear for only traditional form buttons, and get this PR merged.
  • (OR) Let's spend some more time to fine tune some more.

How do you think?

Copy link
Member Author

@a1012112796 a1012112796 May 6, 2022

Choose a reason for hiding this comment

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

Either is fine to me:

  • Make the CSS name in this PR clear for only traditional form buttons, and get this PR merged.
  • (OR) Let's spend some more time to fine tune some more.

How do you think?

how about limit it by this way?

Suggested change
// loading-button this logic used to prevent push one form more than one time
$(document).on('click', '.button.loading-button', function (e) {
const $btn = $(this);
if ($btn.hasClass('loading')) {
e.preventDefault();
return false;
}
// loading-button this logic used to prevent push one form more than one time
// like double create new issue , pull request or comments
$(document).on('click', 'form .button.loading-button', function (e) {
const $btn = $(this);
if ($btn.hasClass('loading')) {
e.preventDefault();
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

loading-button is not a proper name ......

If you are reading some code, when you see a button called loading-button, you might think it is a button which is loading something. BUT, the purpose of the class is to make the button can only submit the form once.

I would suggest to use a clear name to reduce misunderstanding.

@wxiaoguang

This comment was marked as off-topic.

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

it should be fixed to prevent button to submit form using keyboard

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 6, 2022

it should be fixed to prevent button to submit form using keyboard

Oh sorry, maybe I didn't say clearly. To be clear:

  • enter on the target button triggers click, it can be handled, see the demo : https://jsfiddle.net/L0r4yxtw/1/ , enter on the button won't submit the form.

  • What I mean is, if user do enter on other text input fields in the form (not on the button, in such case the first button's click is triggered), or use our Ctrl+Enter to submit, then the submit might not be handled. It is another case (should it be handled in this PR?)

Is it still a change request?

@lafriks lafriks self-requested a review May 6, 2022 15:41
@lafriks
Copy link
Member

lafriks commented May 6, 2022

Ok, handling form submit could be handled in other PR then imho

return false;
}

$btn.addClass('loading disabled');
Copy link
Contributor

@wxiaoguang wxiaoguang May 6, 2022

Choose a reason for hiding this comment

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

Suggested change
$btn.addClass('loading disabled');
$btn.addClass('loading disabled');
$btn.prop('disabled', true);

Should we use $btn.prop('disabled', true); to disable the button entirely? Then it's not clickable anymore, and we do not need to check .loading anymore, and no need to return false (preventDefault)

Copy link
Member Author

Choose a reason for hiding this comment

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

looks semantic ui has handled it, just add disabled class is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

But what if it is not a Fomantic UI button? For example, what if some one writes a form without .ui class but uses your code?

If you require the developers to use your code in Fomantic UI context, you should change the selector to .ui.button .xxxxxx to make sure the code only works for Fomantic UI.

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.

No blocker from my side.

@6543
Copy link
Member

6543 commented May 7, 2022

🚀

@6543 6543 merged commit 672e5a7 into go-gitea:main May 7, 2022
@6543 6543 changed the title prevent double click new issue/pull/comment button Prevent double click new issue/pull/comment button May 7, 2022
@a1012112796 a1012112796 deleted the double_clicked_button branch May 8, 2022 01:44
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 8, 2022
* giteaofficial/main:
  Delete related PullAutoMerge and ReviewState on User/Repo Deletion (go-gitea#19649)
  Allow custom default merge message with .gitea/default_merge_message/<merge_style>_TEMPLATE.md (go-gitea#18177)
  Allow to mark files in a PR as viewed (go-gitea#19007)
  Auto merge pull requests when all checks succeeded via API (go-gitea#9307)
  Hide private repositories in packages (go-gitea#19584)
  Only show accessible teams in dashboard dropdown list (go-gitea#19642)
  prevent double click new issue/pull/comment button (go-gitea#16157)
  Improve reviewing PR UX (go-gitea#19612)
  [skip ci] Updated translations via Crowdin
  Add Changelog v1.16.7 (go-gitea#19575) (go-gitea#19644)
  Set safe dir for git operations in .drone.yml CI (go-gitea#19641)
  Add missing `sorting` column in `project_issue` table (go-gitea#19635)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* prevent double click new issue/pull/comment button

when network is not good, these button maybe
double clicked, then more than one same issues
pulls or comments will be created. this pull
request will fix this bug.

Signed-off-by: a1012112796 <1012112796@qq.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.