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

Extends the CF7 autofill/memory to include select menus and checkboxes #2098

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

patphg
Copy link
Contributor

@patphg patphg commented Dec 15, 2020

Select menu inputs were plug and play.

Checkbox inputs require a different approach but are also simple to add/retrieve.

I did not investigate the "update matching form fields on other windows/tabs" feature as I don't fully understand how it triggers/works...

Select menus were plug and play.

Checkboxes were also similarly simple to add/retrieve.

I did *not* investigate the "update matching form fields on other windows/tabs" feature as I don't fully understand how it triggers/works.
@patphg patphg requested a review from chrisblakley as a code owner December 15, 2020 20:38
Copy link
Owner

@chrisblakley chrisblakley left a comment

Choose a reason for hiding this comment

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

Here are my notes

assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
assets/js/nebula.js Outdated Show resolved Hide resolved
@patphg
Copy link
Contributor Author

patphg commented Dec 15, 2020

Hold on for this, I discovered that my local storage entries were cached in, so I need to write a bit more to get them setting properly the initial page visit. I must have had it working at some point or else the entries wouldn't exist to be changed... few more edits I guess.

@patphg
Copy link
Contributor Author

patphg commented Dec 16, 2020

I went down a whole rabbithole and I'm starting over, so next time I push code take a look at it fresh, basically.

@patphg
Copy link
Contributor Author

patphg commented Dec 16, 2020

This has been rewritten to add checkboxes and radio buttons now. I'll try to explain the general idea of what's happening:

On interaction, radios, text, selects, checkboxes, etc. are stored as their val(), while checkboxes are stored as their value only if they have been checked. Else they are stored as null. (The current method will not work if an input is set to disabled but also selected by default or otherwise contains a value. This might not be a realistic problem but just want to note it.)

To sum it up, on page load, logic happens and radio buttons and checkboxes are set to checked if their val() matches what's stored in data.

Multi-window/multi-tab updating is working for these input types, too. The AJAX submit fail condition is also updated.

This new script removes some .trigger() methods which are no longer needed, and some were causing infinite loops by triggering recurring local storage updates when multiple windows were open. This new code might fix the comment about IE 11 infinite loop? I didn't check into it further.

Finally, this now adds a class .nebula-autofilled to the input if it has been autofilled on page load (but not on cross-window update) which might be useful one day.

@patphg patphg requested a review from chrisblakley December 16, 2020 03:15
Copy link
Owner

@chrisblakley chrisblakley left a comment

Choose a reason for hiding this comment

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

My notes– just minor stuff.

jQuery(this).val(thisLocalStorageVal).trigger('keyup');
if ( thisLocalStorageVal !== null ){
if ( jQuery(this).attr('type') === 'checkbox' || jQuery(this).attr('type') === 'radio' ){
jQuery(this).prop('checked', (jQuery(this).val() === thisLocalStorageVal));
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a huge fan of the conditional within the setting part of this itself. Let's set a variable for that with a comment explanation.

if ( !jQuery(this).hasClass('do-not-store') && !jQuery(this).hasClass('.wpcf7-captchar') && thisLocalStorageVal && thisLocalStorageVal !== 'undefined' && thisLocalStorageVal !== '' ){
if ( jQuery(this).val() === '' ){ //Don't overwrite a field that already has text in it!
jQuery(this).val(thisLocalStorageVal).trigger('keyup');
if ( thisLocalStorageVal !== null ){
Copy link
Owner

Choose a reason for hiding this comment

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

We're checking if thisLocalStorageVal is not null, undefined, or an empty string above (line 2734). Is this conditional redundant?

if ( thisLocalStorageVal !== null ){
if ( jQuery(this).attr('type') === 'checkbox' || jQuery(this).attr('type') === 'radio' ){
jQuery(this).prop('checked', (jQuery(this).val() === thisLocalStorageVal));
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd love if we could find a way to not need this else... it seems tantalizingly close, but it may be necessary.

if ( !jQuery(this).hasClass('do-not-store') && !jQuery(this).hasClass('.wpcf7-captchar') ){
jQuery(this).val(localStorage.getItem('cf7_' + jQuery(this).attr('name'))).trigger('keyup');
var thisLocalStorageVal = localStorage.getItem('cf7_' + jQuery(this).attr('name'));
if ( thisLocalStorageVal !== null ){
Copy link
Owner

Choose a reason for hiding this comment

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

Could this simply be:

Suggested change
if ( thisLocalStorageVal !== null ){
if ( thisLocalStorageVal ){

var thisLocalStorageVal = localStorage.getItem('cf7_' + jQuery(this).attr('name'));
if ( thisLocalStorageVal !== null ){
if ( jQuery(this).attr('type') === 'checkbox' || jQuery(this).attr('type') === 'radio' ){
jQuery(this).prop('checked', (jQuery(this).val() === thisLocalStorageVal));
Copy link
Owner

Choose a reason for hiding this comment

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

Same note as above, let's throw the string to boolean conversion into a variable (with comment explanation) instead of doing it all inline. We're going to forget what this does after a while.

if ( jQuery(this).attr('type') === 'checkbox' || jQuery(this).attr('type') === 'radio' ){
jQuery(this).prop('checked', (jQuery(this).val() === thisLocalStorageVal));
} else {
jQuery(this).val( thisLocalStorageVal );
Copy link
Owner

Choose a reason for hiding this comment

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

Just to be consistent with the rest of Nebula:

Suggested change
jQuery(this).val( thisLocalStorageVal );
jQuery(this).val(thisLocalStorageVal);

}
});
});

//Clear localstorage when AJAX submit fails (but submit still succeeds)
if ( window.location.hash.indexOf('wpcf7') > 0 ){
if ( jQuery(escape(window.location.hash) + ' .wpcf7-mail-sent-ok').length ){
jQuery(escape(window.location.hash) + ' .wpcf7-textarea, ' + escape(window.location.hash) + ' .wpcf7-text').each(function(){
jQuery(escape(window.location.hash) + ' .wpcf7-textarea, ' + escape(window.location.hash) + ' .wpcf7-text, ' + escape(window.location.hash) + ' .wpcf7-checkbox input, ' + escape(window.location.hash) + ' .wpcf7-radio input, ' + escape(window.location.hash) + ' .wpcf7-select, ').each(function(){
Copy link
Owner

Choose a reason for hiding this comment

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

Since we're adding the escape() a bunch, let's set it to a variable above 2780 and then use that variable as needed.

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