fix(shortcode): load modal forms in form grid via AJAX #2951 #2955

Sidsector9 · 2018-03-27T15:09:48Z

Phase - 1

All the forms were loaded and hidden via CSS on the form grid page.

Phase - 2

@kevinwhoffman suggested instead of loading every form at once, we should instead use AJAX and fetch only the form that is required. This will also speed up the load time of the form grid page.

After some research here and here, it was determined that implementing AJAX at the current stage of Give Core, it might break certain add-ons.

Phase - 3

This phase was reverting back to phase - 1 and making some additional fixes as suggested by @kevinwhoffman and @ravinderk here and here.

Future plans

We will be implementing this with Ajax after the Javascript files have been refactored to the requirements.

How Has This Been Tested?

Manually tested

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

Earlier every grid item had form hidden using CSS, now the hidden form is instead fetched using AJAX.

ravinderk · 2018-03-27T15:22:21Z

@Sidsector9

Can you confirm that you tested this with recurring addon + multiple payment gateways?
Also, did you test a case where the same form exist twice on the page?

DevinWalker · 2018-03-27T17:01:21Z

assets/src/css/frontend/donation-grid.scss

@@ -263,3 +259,125 @@
 .my-mfp-slide-bottom.mfp-removing.mfp-bg {
 	opacity: 0;
 }
+
+.white-popup {


@Sidsector9 I feel these magnific styles should exist outside of the grid stylesheet within their own since likely these styles will be used in other places as well and finding them here seems difficult for developer who don't know. @kevinwhoffman what are your thoughts?

I agree. Sid and I are going to meet and review ITCSS and I think reworking these class names and locations would be a good first task.

DevinWalker · 2018-03-28T18:43:51Z

@Sidsector9 please reply to @ravinderk's query above asap. I'm also going to request a review by @ravinderk

@kevinwhoffman also wants to do a call with @Sidsector9 regarding ITCSS. I'm moving this from the "Ready for QA" pipeline.

Sidsector9 · 2018-03-29T10:22:18Z

Call Summary
Participants: @ravinderk and @Sidsector9
Topic: Events don't fire of elements loaded via AJAX after DOM is loaded.

Result: We are fetching HTML of the form to show inside a popup. Till here, it works well.
The problem arises when the fetched HTML contains elements which are attached to event handlers. Since this fetched HTML was not originally part of the DOM and appended via AJAX after the DOM was loaded, the event handlers could not be attached to the the elements, unless, they're already being handled via event delegation.

For example, I wasn't able to change the payment method because the radio buttons were not attached to the event handlers since they were loaded after the DOM was loaded. So I made a small change (see here), where I was able to make it work via event delegation. Then came another problem where the the CC number wasn't being formatted because Give.form.fn.field.formatCreditCard() was fired on give_gateway_loaded event.

@ravinderk and I concluded that the existing JS must be rewritten in a way that we don't miss out on anything while working on this feature OR we should only implement the redirect feature and have the modal feature for future release.

CC: @DevinWalker @kevinwhoffman

kevinwhoffman · 2018-03-30T03:15:24Z

It sounds like there will be a lot of hurdles in the way of making Give forms load entirely through AJAX. We also have to consider all of our add-ons' functionality and ensure that they get initiated correctly when the form does not exist on the page at load time.

@ravinderk Can you please provide a story point estimate of what it would take to get Give forms to load via AJAX, including compatibility with each of our add-ons?

ravinderk · 2018-04-04T10:14:20Z

@kevinwhoffman @DevinWalker

Suggestion:
I think we can output HTML of form in the hidden mode for modal view.
I can see backward compatibility issue which we will face when we will update logic to render modal with ajax. My recommendation is to change the name from modal to something and preserve it for ajaxify modal display_style which will help us to deprecated functionality without compromising shortcode attribute name.

Question:

@kevinwhoffman can we change default display_style value from redirect to other because we can not force the admin to enable single donation form page if not doing yet.

DevinWalker · 2018-04-04T18:36:29Z

@ravinderk - yes it's fine to switch the default display_style.

kevinwhoffman · 2018-04-05T03:32:27Z

Slack Call Summary

Participants: @ravinderk @kevinwhoffman
Topic: Discuss backwards compatibility concerns with renaming modal
Result: Ravinder made a good point about third-party code possibly breaking in the future if we change the forms to load via AJAX. Therefore we should treat the AJAX and non-AJAX display styles as separate attributes.

Set the default display_style to modal_reveal which will load the hidden forms on the page and reveal them when the card is clicked. This is the current behavior as seen in the last video I recorded.

In the future, we may introduce a truly modal display style which will load the forms via AJAX. Do not worry about including this for now.

Remaining Tasks

Set default display_style to modal_reveal and update logic accordingly.

This is a revert commit. The forms are shown/hidden instead of using AJAX to load the form.

display_style "modal" is changed to "modal_reveal"

ravinderk

@DevinWalker @kevinwhoffman please review this pr and merge it. Working fine for me.

DevinWalker · 2018-04-11T03:01:09Z

Review Findings

@Sidsector9 it's working well but I found a plugin conflict with Currency Switcher add-ons. Please work with @emgk and @ravinderk to resolve the following:

Very noticeable delays in load time needs resolving
Ensure modal window loads properly
Ensure feature image displays as expected

See my video while I debugged for more info: https://goo.gl/6HT84Z

After I finished recording I isolated the issue to the Currency Switcher add-on.

Sidsector9 · 2018-04-11T07:21:02Z

Call Summary

Participants: @emgk and @Sidsector9
Topic: Form data don't display in modal popup due to Currency Switcher add-on
Result: Enabling Currency Switcher sets form id to 0 which is why the popup doesn't show up the data. @emgk will be fixing this issue.

Sidsector9 · 2018-04-11T12:11:33Z

@DevinWalker, 2/3 points you mentioned are being addressed below:

Ensure modal window loads properly

@emgk has fixed the bug in Currency Switcher add-on and that made the form load correclty

Ensure feature image displays as expected

The feature image too loads correctly after the above said fix

Kindly test this from your end and let me know if further improvements need to be done.

# Conflicts: # assets/src/css/frontend/give-frontend.scss # assets/src/css/frontend/modal.scss # includes/shortcodes.php

ravinderk · 2018-04-12T16:14:42Z

@Sidsector9 I think do not care about #3030 issue for on form grid page. Currently auto open modal on basis of URL param does not work with a form with display_style set to button.

We have to work from the ground to make it compatible with display_style=button also. So ignore it for now.

@DevinWalker What do you think

If you visit a URL directly with query parameters 'form-id' and 'payment-mode' for a form with display type 'button', the popup will pop up.

…ateway or not

new fn give_get_donation_donor_email

Stop using Give_Payment and Give_Donor to improve performance on frontend

…sting_payment fn

…bels.

fix impress-org#2998

…issue-2951 # Conflicts: # assets/src/css/frontend/modal.scss

fix(shortcode): Use AJAX to render donation form

4defead

Earlier every grid item had form hidden using CSS, now the hidden form is instead fetched using AJAX.

DevinWalker reviewed Mar 27, 2018

View reviewed changes

DevinWalker requested a review from ravinderk March 28, 2018 18:43

Sidsector9 added 2 commits March 29, 2018 01:02

fix(shortcode): Add event delegation to load proper gateway

37403b9

fix(shortcode): Move css to modal.scss file

214a31d

Merge branch 'release/2.1' into issue-2951

02e09ca

Sidsector9 and others added 3 commits April 5, 2018 15:22

fix(shortcode): Remove form load by AJAX

170cb21

This is a revert commit. The forms are shown/hidden instead of using AJAX to load the form.

fix(shortcode): rename display style

11e311f

display_style "modal" is changed to "modal_reveal"

fix(shortcode): undo js chnage for modal

3407cc8

ravinderk approved these changes Apr 5, 2018

View reviewed changes

ravinderk approved these changes Apr 6, 2018

View reviewed changes

Merge branch 'release/2.1' into issue-2951

5191b5b

perf(shortcode): Restructure SCSS and remove redundant code

11324d4

Sidsector9 and others added 2 commits April 12, 2018 18:58

Merge branch 'release/2.1' into issue-2951

acee2a3

Merge branch 'release/2.1' into issue-2951

bf0f01e

# Conflicts: # assets/src/css/frontend/give-frontend.scss # assets/src/css/frontend/modal.scss # includes/shortcodes.php

ravinderk approved these changes Apr 12, 2018

View reviewed changes

Sidsector9 and others added 3 commits April 13, 2018 11:14

Merge branch 'release/2.1' into issue-2951

e8bfca3

fix(modal): Add popup for display type button

5226763

If you visit a URL directly with query parameters 'form-id' and 'payment-mode' for a form with display type 'button', the popup will pop up.

perf(code): stop using Give_Payment in helper fns

ee6bbc1

ravinderk and others added 25 commits April 16, 2018 21:04

feat(donation): add helper fn to check if donation process by given g…

631e89a

…ateway or not

fix(donation): add helper function to get donor email

f4dc85b

new fn give_get_donation_donor_email

fix(donation): refactor email tag code

1a89076

Stop using Give_Payment and Give_Donor to improve performance on frontend

fix(donation): refactor email tag code

916a9cd

Stop using Give_Payment and Give_Donor to improve performance on frontend

refactor(donation): remove payment meta query from receipt

ee061a6

refactor(donation): remove fourth param from filter

c221b50

fix(admin-settings): check if fields exist in array or not

74059e1

refactor(donation): remove Give_Payment usage from give_check_for_exi…

7832369

…sting_payment fn

fix(donation): remove $this from fn to prevent fatal error

7aa364f

fix(admin-settings): check if fields exist in array or not pass2

2090686

fix(donation): get donor email if donation email empty

91ba617

fix(donation): pass default deprecated param to prevent fatal error

d794398

fix(translation): used html entities for currency symbol for admin la…

9da8f34

…bels.

fix(donation-form): do not output formatted min max value .

221bbf0

fix(phpunit): remove extra space from input tag

d6d661d

fix(translation): improved error message for better translation.

61518fd

fix(admin-update): make string more translatable

28b36dd

styles(code): update fn description

a7a6734

fix(admin-tool): make import page as default impress-org#3015

0ef56ec

fix(admin-tool): change tab order impress-org#3015

24bfff3

fix(admin-tool): remove php notice from system info page

86b60e5

fix impress-org#2998

fix(admin-form): update field title for custom amount range

3a7bf9c

fix(modal): Fix modal content when display option=button

3920d24

fix(modal): modal popup through URL parameters

225116e

fix(shortcode): Add proper redirection on failure

5311f01

Sidsector9 force-pushed the issue-2951 branch from 7b357a0 to 5311f01 Compare April 16, 2018 15:48

Sidsector9 and others added 2 commits April 16, 2018 21:21

Merge branch 'release/2.1' into issue-2951

447838c

Merge branch 'issue-2951' of https://github.com/Sidsector9/Give into …

cebe4f7

…issue-2951 # Conflicts: # assets/src/css/frontend/modal.scss

DevinWalker merged commit e57ab20 into impress-org:release/2.1 Apr 17, 2018

ravinderk mentioned this pull request May 2, 2018

fix(form): getting js error when get back to page with form grid shortcode #3019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(shortcode): load modal forms in form grid via AJAX #2951 #2955

fix(shortcode): load modal forms in form grid via AJAX #2951 #2955

Sidsector9 commented Mar 27, 2018 •

edited

ravinderk commented Mar 27, 2018 •

edited

DevinWalker Mar 27, 2018

kevinwhoffman Mar 27, 2018

DevinWalker commented Mar 28, 2018

Sidsector9 commented Mar 29, 2018 •

edited

kevinwhoffman commented Mar 30, 2018

ravinderk commented Apr 4, 2018

DevinWalker commented Apr 4, 2018

kevinwhoffman commented Apr 5, 2018 •

edited by Sidsector9

ravinderk left a comment

DevinWalker commented Apr 11, 2018

Sidsector9 commented Apr 11, 2018

Sidsector9 commented Apr 11, 2018

ravinderk commented Apr 12, 2018

fix(shortcode): load modal forms in form grid via AJAX #2951 #2955

fix(shortcode): load modal forms in form grid via AJAX #2951 #2955

Conversation

Sidsector9 commented Mar 27, 2018 • edited

Phase - 1

Phase - 2

Phase - 3

Future plans

How Has This Been Tested?

Checklist:

ravinderk commented Mar 27, 2018 • edited

DevinWalker Mar 27, 2018

Choose a reason for hiding this comment

kevinwhoffman Mar 27, 2018

Choose a reason for hiding this comment

DevinWalker commented Mar 28, 2018

Sidsector9 commented Mar 29, 2018 • edited

kevinwhoffman commented Mar 30, 2018

ravinderk commented Apr 4, 2018

DevinWalker commented Apr 4, 2018

kevinwhoffman commented Apr 5, 2018 • edited by Sidsector9

Remaining Tasks

ravinderk left a comment

Choose a reason for hiding this comment

DevinWalker commented Apr 11, 2018

Review Findings

Sidsector9 commented Apr 11, 2018

Call Summary

Sidsector9 commented Apr 11, 2018

ravinderk commented Apr 12, 2018

Sidsector9 commented Mar 27, 2018 •

edited

ravinderk commented Mar 27, 2018 •

edited

Sidsector9 commented Mar 29, 2018 •

edited

kevinwhoffman commented Apr 5, 2018 •

edited by Sidsector9