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

Add functionality to turn popups on/off easily. #544

Closed
danieliser opened this issue Mar 23, 2018 · 19 comments · Fixed by #877
Closed

Add functionality to turn popups on/off easily. #544

danieliser opened this issue Mar 23, 2018 · 19 comments · Fixed by #877

Comments

@danieliser
Copy link
Member

Possible options:

  • Piggy back on post_status. But this may not be an option. see below.
  • Treat a setting value as a condition, similar to the disable on mobile options. If "Off" conditions or is_loadable fails outright.

Draft may not be an option per facebook group comments:

"The problem with using draft is that the inexperienced user doesn't always understand or forgets. Also if a user makes an update, it will switch from draft to published. Announcer https://wordpress.org/plugins/announcer/ has an option to turn an announcement on and off and also shows the status in the list view."

Not sure the viability of adding a true on/off parameter to use as this could muck up queryies

@fpcorso
Copy link
Contributor

fpcorso commented Jan 31, 2020

@danieliser Were you still considering this? Not sure if this is needed or if we could find a better way to communicate the possibility of switching to draft or how it works.

@danieliser
Copy link
Member Author

@fpcorso your call

@fpcorso
Copy link
Contributor

fpcorso commented Mar 18, 2020

Have had several more people ask for the ability to turn popups on or off. The most recent was this feedback in a review: https://wordpress.org/support/topic/easy-to-learn-6/#post-12558241

@fpcorso
Copy link
Contributor

fpcorso commented Mar 24, 2020

I like how Beaver Builder handles it which just switches the post from draft to published and back when clicked. Of course, just using draft as off runs into possible issues that you described before but, with the on/off, at least users would be able to quickly tell if it's on or not.

image

@danieliser
Copy link
Member Author

Yea I like thier button, but the draft thing still applies I think.

@fpcorso
Copy link
Contributor

fpcorso commented Mar 28, 2020

@danieliser , while the draft issues you described could potentially be an issue, I haven't seen a better approach. I've noticed quite a few plugins lately use a similar button to BB that is tied to published/draft status so that is becoming quite the norm now.

I haven't seen this get asked about as often as some of our other issues but still just enough that we probably want to see if this is something we want to narrow down on to solve or not. For me, I think having a simple on/off switch would make managing popups much more intuitive to the end-user.

@danieliser
Copy link
Member Author

@fpcorso - simple enough to set a new postmeta field and link the button to that. We simply rewrite is_loadable to check that meta value before it processes conditions.

Simple enough, just needs an AJAX handler and some JS for the toggle.

@fpcorso
Copy link
Contributor

fpcorso commented Mar 28, 2020

@danieliser , would having another postmeta field to query with add slowdowns to the queries? Would that make things messy for our queries overall?

You had mentioned originally that may muck up queries so I thought you were against that strategy.

@danieliser
Copy link
Member Author

danieliser commented Mar 28, 2020

@fpcorso Not at all. We have 4 queries max ever.

If you use asset caching that goes to 2 queries max.

We query for all popups + meta (2 queries), and popup_themes + meta if asset caching is off.

Every page load queries all popups published popups. Testing if they should or shouldn't load in the page is handled by conditional processing.

If you use object caching like Redis or Memcache for queries, then 0 queries after the first page load and the query cache is primed.

So adding another meta field won't hurt anything in terms of performance, especially if its nothing more than 1 or 0

@fpcorso fpcorso added this to the v1.11 milestone Apr 15, 2020
@fpcorso fpcorso modified the milestones: v1.11, v1.12 Apr 30, 2020
@fpcorso
Copy link
Contributor

fpcorso commented May 11, 2020

Had another person ask for this: https://wordpress.org/support/topic/great-easy-to-use-popup-plugin

@fpcorso
Copy link
Contributor

fpcorso commented Jun 9, 2020

Had another person ask for this:
image

@fpcorso
Copy link
Contributor

fpcorso commented Jul 1, 2020

Had another person ask for this:
image

@fpcorso
Copy link
Contributor

fpcorso commented Jul 17, 2020

Had another person ask for this in a recent customer survey.

@fpcorso
Copy link
Contributor

fpcorso commented Jul 17, 2020

@danieliser Just to recap our conversation on this: You were stating we should have a new post_meta field for the popup being "on" or "active". Do you think it would be too much for admins to check both published and active to ensure a popup is showing?

For the actual mechanism, are we going with the on/off switch on the "All Popups" page? Should there also be some mechanism on the edit popup screen?

When the popup is published, it should automatically switch to "active" or "on", right? What if the user turned it off and switched to draft and then, published it after making changes? Would we always want to switch to "on" upon publishing the popup?

@danieliser
Copy link
Member Author

@fpcorso - We either are storing a different data point, or just making a new interface for draft/published. But then we have to account for other possible status as well.

  1. There likely should be a way to change it on the editor screen.

  2. I'd say it doesn't even appear until a popup is published. We won't load a non published popup in our queries anyways.

@fpcorso
Copy link
Contributor

fpcorso commented Sep 9, 2020

@danieliser We need to make a decision on whether this is a different post_meta or if it's a better interface for draft/published. In terms of usability, fewer options are always better for use and support. And, keeping it only built on draft/published would require less work to ensure all of our queries and extensions work as expected after the change.

Of course, there are several reasons for us to consider going with the new different data point, as you mentioned in previous comments.

Which way are you leaning?

@danieliser
Copy link
Member Author

@fpcorso Off the top of my head, there are no queries that would need adjusted to add a new meta point.

I think using post_status comes with its own set of potential flaws and drawbacks and will absolutely lead to support issues at some point.

Our only major query is to load all published popups and their meta.

After that each published popup's conditions are checked, if the non-advanced conditions pass it is enqueued and rendered in the footer.

So I think using meta is appropriate here and fully controllable.

  1. Add new interface with toggle switch for on/off.
  2. Add new AJAX process to handle changes to the status immediately from either All Popups or Popup Editor. Maybe even from the front end at some point.
  3. Modify the PUM_Model_Popup::is_loadable method to return false early if the post_meta value === 'off'.
  4. If the post_meta value is on or false|null (not set) then it should process conditions normally.

That effectively makes the toggle a short circuit for is_loadable and essentially the smoothest, most effective place to implement it.

Since we already have all the meta loaded, there won't be extra queries or any overhead this way.

fpcorso added a commit that referenced this issue Sep 11, 2020
fpcorso added a commit that referenced this issue Sep 11, 2020
@fpcorso fpcorso linked a pull request Sep 11, 2020 that will close this issue
9 tasks
fpcorso added a commit that referenced this issue Sep 11, 2020
fpcorso added a commit that referenced this issue Sep 11, 2020
fpcorso added a commit that referenced this issue Sep 11, 2020
fpcorso added a commit that referenced this issue Sep 11, 2020
@fpcorso
Copy link
Contributor

fpcorso commented Sep 15, 2020

@danieliser Just remembered that we wanted to only show the toggle if the popup is published, which makes sense. Was an easy enough change. But, not sure on what to put in the column when the popup is not published. We probably shouldn't leave it empty or the admin may not know why it's empty.

In the image, I tried using Popup not published. Do you have any ideas for something better to go there?

image

@danieliser
Copy link
Member Author

@fpcorso I love this change. It will actually highlight one potential issue that people overlook when they contact support for "my popup doesn't work".

So putting Popup is not published also serves to reduce support and make it more intuitive.

Brilliant.

As for the text, I think its pretty spot on. We could maybe make a link below it to "Publish It Now" later if we wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants