Updated jQuery to Javascript for button disable on submit

[padmasreegade]

Updated jquery to javascript for button disable on submit in commons.js - initDisableSubmitOnClick

[demiankatz]

Thanks, @padmasreegade! Unfortunately, this doesn't seem to be working correctly yet. I suspect that part of the problem is that it is fairly difficult to test this functionality. I have created a simple test which should help with troubleshooting.

If you edit themes/bootstrap3/templates/search/home.phtml and add this code at the bottom:

<form id="testSubmitForm" data-disable-on-submit><input type="submit"></form>
<?php
$script = <<<JS
  document.getElementById('testSubmitForm').addEventListener(
    'submit',
    function (e) {
      e.preventDefault();
      console.log('submitted');
      return false;
    }
  );
JS;
echo $this->inlineScript(\Laminas\View\Helper\HeadScript::SCRIPT, $script, 'SET');
?>

Then when you go to the main VuFind home page in your browser, you will see an extra submit button at the bottom of the main body, above the page footer. This button will just push the word "submitted" to your browser console when you click on it. It has the data-disable-on-submit attribute, so it should only be possible to submit it once per session -- it should get disabled after the first click.

I have confirmed that this works correctly on the dev branch, but when I try the test on this branch, the button does not get disabled.

A few thoughts on things you might want to change:

1.) There could be multiple forms on the page that need this treatment, so I think you might want to use document.querySelectorAll('[data-disable-on-submit]') to get a list of elements that need modification.

2.) I think you may need to use addEventListener in a similar fashion to my code above to attach the event without risking interfering with other existing listeners.

3.) It looks like your disableSubmit function needs some adjustment -- because it's using querySelector, it will only ever match one element, which might be a problem if a form contains multiple submit buttons. And because it's selecting at the document level, it is not scoped to the form that's being submitted, so it could end up disabling the wrong button in the wrong form.

I hope that this all helps you to make some further progress. Please let me know if I can do anything more to help with the process!

[padmasreegade]

Thank you for your suggestions Demian, I will look into that. Regards, Padmasree Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Demian Katz ***@***.***> Sent: Wednesday, October 25, 2023 2:12:38 PM To: vufind-org/vufind ***@***.***> Cc: Padmasree Gade ***@***.***>; Mention ***@***.***> Subject: [EXTERNAL] Re: [vufind-org/vufind] Updated jquery to javascript for button disable on submit (PR #3181) @demiankatz requested changes on this pull request. Thanks, @padmasreegade<https://github.com/padmasreegade>! Unfortunately, this doesn't seem to be working correctly yet. I suspect that part of the problem is that it is fairly difficult to test this functionality. I have created a simple test which should help with troubleshooting. If you edit themes/bootstrap3/templates/search/home.phtml and add this code at the bottom: <form id="testSubmitForm" data-disable-on-submit><input type="submit"></form> <?php $script = <<<JS document.getElementById('testSubmitForm').addEventListener( 'submit', function (e) { e.preventDefault(); console.log('submitted'); return false; } ); JS; echo $this->inlineScript(\Laminas\View\Helper\HeadScript::SCRIPT, $script, 'SET'); ?> Then when you go to the main VuFind home page in your browser, you will see an extra submit button at the bottom of the main body, above the page footer. This button will just push the word "submitted" to your browser console when you click on it. It has the data-disable-on-submit attribute, so it should only be possible to submit it once per session -- it should get disabled after the first click. I have confirmed that this works correctly on the dev branch, but when I try the test on this branch, the button does not get disabled. A few thoughts on things you might want to change: 1.) There could be multiple forms on the page that need this treatment, so I think you might want to use document.querySelectorAll('[data-disable-on-submit]') to get a list of elements that need modification. 2.) I think you may need to use addEventListener in a similar fashion to my code above to attach the event without risking interfering with other existing listeners. 3.) It looks like your disableSubmit function needs some adjustment -- because it's using querySelector, it will only ever match one element, which might be a problem if a form contains multiple submit buttons. And because it's selecting at the document level, it is not scoped to the form that's being submitted, so it could end up disabling the wrong button in the wrong form. I hope that this all helps you to make some further progress. Please let me know if I can do anything more to help with the process! — Reply to this email directly, view it on GitHub<#3181 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BCM24W3L2NBTYQRGRHMNBQLYBFJBNAVCNFSM6AAAAAA6PUZ2WGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOJXHE3TEMBTGI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[padmasreegade]

Thanks, @padmasreegade! Unfortunately, this doesn't seem to be working correctly yet. I suspect that part of the problem is that it is fairly difficult to test this functionality. I have created a simple test which should help with troubleshooting.

If you edit themes/bootstrap3/templates/search/home.phtml and add this code at the bottom:

<form id="testSubmitForm" data-disable-on-submit><input type="submit"></form>
<?php
$script = <<<JS
  document.getElementById('testSubmitForm').addEventListener(
    'submit',
    function (e) {
      e.preventDefault();
      console.log('submitted');
      return false;
    }
  );
JS;
echo $this->inlineScript(\Laminas\View\Helper\HeadScript::SCRIPT, $script, 'SET');
?>

Then when you go to the main VuFind home page in your browser, you will see an extra submit button at the bottom of the main body, above the page footer. This button will just push the word "submitted" to your browser console when you click on it. It has the data-disable-on-submit attribute, so it should only be possible to submit it once per session -- it should get disabled after the first click.

I have confirmed that this works correctly on the dev branch, but when I try the test on this branch, the button does not get disabled.

A few thoughts on things you might want to change:

1.) There could be multiple forms on the page that need this treatment, so I think you might want to use document.querySelectorAll('[data-disable-on-submit]') to get a list of elements that need modification.

2.) I think you may need to use addEventListener in a similar fashion to my code above to attach the event without risking interfering with other existing listeners.

3.) It looks like your disableSubmit function needs some adjustment -- because it's using querySelector, it will only ever match one element, which might be a problem if a form contains multiple submit buttons. And because it's selecting at the document level, it is not scoped to the form that's being submitted, so it could end up disabling the wrong button in the wrong form.

I hope that this all helps you to make some further progress. Please let me know if I can do anything more to help with the process!

I have added this code in the homepage for testing and it seems to have worked. Please review.

[crhallberg]

As discussed, this looks good once adapted for multiple forms and the style issues are addressed (phing.sh qa-js-and-less).

[demiankatz]

@padmasreegade, thanks for the progress on this; it's looking much better, and I think all of my original feedback has been addressed. I haven't had a chance to test it hands-on yet, but I will try to do that soon.

The only small change I made was to delete some unnecessary blank spaces to reduce the number of diffs reported in the PR. I'd recommend turning on the "remove trailing whitespace" setting in your editor, if you're using an editor that offers that setting.

I've also checked in with @EreMaijala to see if he wants to look at this before it gets merged, since he wrote the original jQuery-based code.

[EreMaijala]

Looks good to me, just a minor detail to change.

[demiankatz]

Thanks, @padmasreegade, this looks good to me, but I'll wait for @EreMaijala to confirm before I merge.

Review has been addressed.