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

remove redundant load of google.com/recaptcha/api.js #12

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

wesleyboar
Copy link

Overview

Remove seemingly redundant call to google.com/recaptcha/api.js.

Testing

  1. Install form plugin via base branch on a CMS.
  2. Enable captcha.
  3. ⓧ Verify console error originating from the script I propose to remove.
  4. Install form plugin via this branch on a CMS.
  5. ✓ Verify console error of relevant previous step does not appear.

A form using this change by me is (as of this PR) currently only available on one public webpage:
https://frontera-portal.tacc.utexas.edu/user-meeting/2022/

⚠️ Although my rough testing proved this unnecessary, someone more familiar with the code should evaluate.

Notes

While testing this plugin in TACC/Core-CMS, I found this call to be redundant.

Removing this block from my apps template also fixed a benign console error.

The 'django-recaptcha' app already loads this with dynamic parameters:
https://github.com/torchbox/django-recaptcha/blob/4921a6c/captcha/templates/captcha/includes/js_v2_checkbox.html#L2

Source: https://github.com/TACC/Core-CMS/pull/500/files#r898443017

While testing this plugin in TACC/Core-CMS, I found this call to be redundant.

Removing this block from my apps template also fixed a benign console error.

The 'django-recaptcha' app already loads this with dynamic parameters:
https://github.com/torchbox/django-recaptcha/blob/4921a6c/captcha/templates/captcha/includes/js_v2_checkbox.html#L2

Source: https://github.com/TACC/Core-CMS/pull/500/files#r898443017
@wesleyboar wesleyboar changed the title remove duplicate load of recaptcha/api.js remove redundant load of google.com/recaptcha/api.js Jul 6, 2022
@avryhof
Copy link
Owner

avryhof commented Jul 8, 2022

Yes, this is certainly redundant because django-recaptcha loads the appropriate javascript depending on the captcha version you choose within its own templates. Good Catch, and this change will appear in the next release.

@avryhof avryhof merged commit eaa6415 into avryhof:master Jul 8, 2022
wesleyboar added a commit to TACC/Core-CMS that referenced this pull request Jul 8, 2022
The removal happens form a merged PR on the plugin:
avryhof/djangocms-forms#12
wesleyboar added a commit to TACC/Core-CMS that referenced this pull request Jul 15, 2022
* feat(tup-230): working form plugin

* docs(tup-230): clarify form submission failure

* feat(tup-230): use jquery version that has ajax

The django-forms(-maintained) plugin expects jquery with ajax features.

Our CMS was using a slim build of jQuery.

The size difference is 17KB, which I consider negligible.

* feat(tup-230): make spam protection available

* feat(tup-230): e-mail template (when e-mail works)

* docs(tup-230): tweak comment in settings

* fix(tup-230): comments & semantic html in template

* fix(tup-230): revert and make some html changes

- less changes form original
- sometimes the original html was better for styling

* feat(tup-230): style form (no design review yet)

Also, no button styling yet.

* chore(tup-230): rename css file, add pointer file

Pointer file mimics what I do for blog CSS, which is also in source.

* chore(settings): tup-308, how to set captcha keys

Ref: TACC/Core-Portal-Deployments@48e6e9c

* noop(settings): tup-308, add new line

* fix(settings): tup-308, use new form css from #500

* fix(settings): tup-308, new form templates by #500

Not a straight clone of #500.

I added a little CSS and a small document to avoid cloning one template.

See:
- default.html.md
- django.cms.forms.css

* noop(settings): tup-308, form settings from #501

* fix(settings): tup-308, css changes from #501

* fix(settings): tup-308, "*" to "(required)"

I use a different solution than #501.

In #501, markup was changed to "(require)":
https://github.com/TACC/Core-CMS/blob/5f63bdf/taccsite_cms/templates/djangocms_forms/form_template/default.html#L27

But here, I do not edit the markup, so no need to clone template:
https://github.com/TACC/Core-CMS/blob/8a46a21/taccsite_cms/templates/djangocms_forms/form_template/default.html.md

* fix(css): tup-308, small form style issues

- bigger gap between single checkbox and input
- add gap between multiple checkbox and input
- align multiple checkbox and input
- simpler, less specific checkbox label selector
- remove undesired label margin*

* This style was ineffectual—good—in 3.6, but "fixed"—bad—when ported.

* fix(css): tup-308, small form style issues

- limit which help text gets margin
- remove ineffectual, unnecessary margin removal
- bigger gap between single checkbox and input
- align single checkbox and input
- add gap between multiple checkbox and input
- align multiple checkbox and input
- remove label margin on last of multiple checkboxes/radios
- simpler, less specific checkbox label selector
- remove undesired label margin*

* This style was ineffectual—good—in 3.6, but "fixed"—bad—when ported.

* docs(styles): typo fix for KSS comment

* fix(form): tup-308 only working export formats

1. Only let users export with supported formats.
2. Update form version to rev. that supports this.

Source: avryhof/djangocms-forms#8

* fix(form): delete unnecessary cloned template

* fix(form): tup-308, clean up templates

- remove unedited cloned template
- update commit in a comment

* fix(form): tup-308, move form export settings

* fix(form): tup-308, remove unused file

I forgot to delete this file earlier.

* fix(form): tup-308, remove redundant script

The removal happens form a merged PR on the plugin:
avryhof/djangocms-forms#12

* chore(core-styles): v0.7.0-beta

* fix(taccsite_custom): get main relevant PRs merged

* docs(form): no commented settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants