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

Show default click trigger classes in the popup editor or click trigger. #584

Closed
danieliser opened this issue Aug 19, 2018 · 16 comments
Closed
Milestone

Comments

@danieliser
Copy link
Member

No description provided.

@fpcorso fpcorso added this to the v1.12 milestone Jun 5, 2020
@fpcorso
Copy link
Contributor

fpcorso commented Jul 10, 2020

@danieliser We added this explainer in 1.11 to the click trigger settings:
image

Is that what this issue is referring to or was there something more you wanted to do here?

@danieliser
Copy link
Member Author

danieliser commented Jul 14, 2020

@fpcorso - Yea that should suffice.

That said now that I've reread that description just now (I did approve it, so I missed it before), I wonder why you chose name over ID. Name's change over time breaking the link.

@fpcorso
Copy link
Contributor

fpcorso commented Jul 15, 2020

@danieliser Yeah, I had copied in the text that was originally near the name field which appears to not have had the ID class with it. Definitely makes sense to use that here. We'll try to switch that out in the next update before closing this then.

@fpcorso
Copy link
Contributor

fpcorso commented Jul 17, 2020

@danieliser That section is added in Triggers.php where I can't easily retrieve the ID for the current popup being edited. I temporarily switched to popmake-{popup-ID} but it would be better to show the actual ID there.
https://github.com/PopupMaker/Popup-Maker/blob/develop/classes/Triggers.php#L157

We could add an empty HTML element there, such as popmake-<span id="click-trigger-id">{popup-ID}</span>. Then, add some JS to our editor JS to replace that with the popup ID.

Can you think of a more efficient way to do it?

@danieliser
Copy link
Member Author

Yea I would use a placeholder and JS to swap it based on the post_ID field value.

@fpcorso fpcorso changed the title Show default click trigger classes inthe popup editor or click trigger. Show default click trigger classes in the popup editor or click trigger. Jul 29, 2020
@fpcorso
Copy link
Contributor

fpcorso commented Jul 29, 2020

@danieliser Set the field to have a span placeholder and have some JS to swap it. However, the click settings template for the field gets regenerated every time you add a trigger. So, when you click the add trigger button, it regenerates the template, thus showing the placeholder instead of the new value I swapped it with. I'm assuming this is done to populate the fields with the correct values but it makes it difficult to swap out the placeholder now.

Alternatively, I could swap out the placeholder when the click trigger settings open instead. But, there doesn't seem to be a good place to do that in the triggers.js file.

fpcorso added a commit that referenced this issue Jul 29, 2020
…ID values

Also includes some code qualtiy improvements from ESLint and PHPCS.

Issue #584
@danieliser
Copy link
Member Author

@fpcorso Try pumInit JS event.

I would do it like this for now.

<span class="pum-default-click-trigger-classes"></span>

And inside the above JS event something like this.

jQuery('.pum-default-click-trigger-classes:not(.initialized)').each(function () {
    var $this = jQuery(this);
    $this.addClass('initialized');
    $this.text('set it to whatever here');
});

Will fire off every time a form is re-rendered.

@fpcorso
Copy link
Contributor

fpcorso commented Aug 20, 2020

@danieliser That code doesn't work. The issue wasn't finding or changing the text. It's that the plugin completely recreates the template when the trigger is being added.

Running that code above changes the span text correctly during pageload or puminit. But, as soon as the trigger button is clicked, the template is overwritten to use the default empty span again. This occurs during the form method here: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/triggers.js#L134

We could add some code after that to change the span. The onsubmit event handler is added after that too so it might make sense to add code affecting the trigger settings modal there.

@danieliser
Copy link
Member Author

danieliser commented Aug 20, 2020

@fpcorso Sorry your right, I was mistaken, it seem the proper event name is pum_init for some reason: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/triggers.js#L304

That event is triggered on every redraw of a form intentionally. Because of that you must "initialize" elements in a detectable way. That is why it uses :not(.initialized) in the selector and $this.addClass('initialized'); in the handler because it will run regularly and we don't want to keep reiniting things that didn't get redrawn.

But this way if the form is redrawn, it will .trigger('pum_init') which will then trigger your callback to look for fields that need to be changed.

Now, you may want to use a different designation class than initialized, for example the select2 fields use pumselect2-initialized because they may also have something else that needs initialized on that same field separately.

@fpcorso
Copy link
Contributor

fpcorso commented Aug 20, 2020

@danieliser Ah, I thought the pum_init type events only fire when all of PM is initialized. I didn't realize we also have one firing each time after the form modal is recreated. I'll need to play with that to see if I can find it and work with that.

Where is that called? I didn't see it in the modal or template JS files which are the only thing called during that form method?

@danieliser
Copy link
Member Author

@fpcorso Check the render method for forms: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/general/plugins/forms.js#L484

It triggers on the form itself, but like I mentioned in my PR review today, events bubble up to document, so we catch it there.

@fpcorso
Copy link
Contributor

fpcorso commented Aug 20, 2020

@danieliser I don't think we ever call any methods from forms.js here. The method I linked to (triggers.form) only calls modals.reload and templates.modal. Neither of which calls any methods from forms.js.

Here is where the modal gets created in triggers.js:
https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/triggers.js#L134

Which calls templates.modal which is just a quick wrapper for templates.render which only calls wp.template and does some preparations before returning it: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/general/plugins/templates.js#L8

All that the modals.reload method does is remove the old HTML and appends the new HTML: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/general/plugins/modals.js#L140

That said, at the very end of that method, it triggers a pum_init event so the idea should still work.

Unfortunately, I can't get it to work for some reason. pum_init event is not firing when opening and going through the modals. I tried using a similar version to the code above but it didn't successfully change the span. I tried attaching a simple console.log('Yes'); to pum_init and it only fired when I called it manually and did not fire when adding or editing triggers for some reason. Still looking through it.

@fpcorso
Copy link
Contributor

fpcorso commented Aug 20, 2020

@danieliser Nevermind that last paragraph. I had a type in my code 🤦‍♂️

@fpcorso
Copy link
Contributor

fpcorso commented Aug 20, 2020

@danieliser So, it does work, but pum_init fires many times throughout the course of creating a popup. My code ran a dozen times by the time I finished testing creating a popup. Is that sufficient or should we go back to the original place I suggested of right after the trigger settings modal gets created?

@danieliser
Copy link
Member Author

@fpcorso Its effecient as long as your code properly bails early. That is what the :not(.initialized) & .addClass('initialized') handle.

Again make a new -initialized class for this.

fpcorso added a commit that referenced this issue Sep 1, 2020
Set up the popup-{popup-ID} to be dynamically replaced with the actual popup ID to make it easier for copying and pasting.

Issue #584
@fpcorso
Copy link
Contributor

fpcorso commented Sep 1, 2020

@danieliser Got this working correctly now:
image

@fpcorso fpcorso closed this as completed Sep 1, 2020
@fpcorso fpcorso mentioned this issue Sep 29, 2020
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

No branches or pull requests

2 participants