pluginkollektiv / cachify

Smart but efficient cache solution for WordPress. Use DB, HDD, APC or Memcached for storing your blog pages. Make WordPress faster!
https://wordpress.org/plugins/cachify/
GNU General Public License v2.0
101 stars 32 forks source link

Inform user on cache clear in admin bar #257

Closed angcl closed 2 years ago

angcl commented 2 years ago

In this pull request, a REST API endpoint is registered through which the cache can be flushed. The Flush site cache button in the admin bar executes a JavaScript function that makes an HTTP request to this endpoint. The user receives an indication whether the request was successful via color coding (currently green and red - possibly bad for color vision deficiency - should be reconsidered).

I would be very happy about feedback on this.

Closes #100, Closes #102

angcl commented 2 years ago

@florianbrinkmann @bueltge Thank you very much.

@florianbrinkmann and I worked together and changed the way the admin bar icon notifies the success or failure of the AJAX-request. We removed that the icon changes the color to green or red. The dashboard-trash icon is replaced by the dashboard-yes-alt or dashboard-dismiss icon instead, based on success/failure state. After 2s it reverts back to the trash icon. Meanwhile it is not possible to fire another clear cache with the admin bar item.

Now the link works as before if e.g. JavaScript was disabled. We added that the dashicon is dashboard-yes-alt after a cache action, so that the user nevertheless gets informed.

At least I extracted the css and js into seperate files.

bueltge commented 2 years ago

Great stuff @angelocali i would leave small notes:

angcl commented 2 years ago

@bueltge Nice to hear you like it. I renamed it to REST_NAMESPACE. I'm not quite sure if I understood you correctly with the wp_register_style part. Now I'm registering the styles and scripts in init first before enqueueing it. Can you second that you meant this?

bueltge commented 2 years ago

In case of the constant. A constant is global visible, usable and speak for himself. Outside this development, if I read the name REST_NAMESPACE - what is the task of this constant? So in my opinion it should have a name that describe his task. You store the version of the Cachify version for REST, maybe the name should go in this direction, like CACHIFY_REST_VERSION.

Short: Write code for humans 👩👨, not only for machines. helpful: https://ahmedmahmoud-eltaher.medium.com/clean-code-best-practice-for-naming-part-1-f67ebe8c0894

stklcode commented 2 years ago

Outside this development, if I read the name REST_NAMESPACE - what is the task of this constant?

At least it’s bound to the class here, so typically called or imported as Cachify::REST_NAMESPACE which is as precise as I’d personally like to read.

Of course it might be even more specific like Cachify::CACHIFY_WP_REST_API_URL_PREFIX_V1 or something reasonable in between. (for …_VERSION I would expect values like v1 or 1)

Btw. the term “namespace“ is commonly used in the WP API docs (https://developer.wordpress.org/rest-api/extending-the-rest-api/routes-and-endpoints/#namespaces)


The PHPdoc comment could be less generic though. It states “endpointS“ although it’s bound to only a single value. Meaningful along with the code, in standalone docs or IDE suggestions REST_ROUTE_FLUSH is omitted.

florianbrinkmann commented 2 years ago

At least it’s bound to the class here, so typically called or imported as Cachify::REST_NAMESPACE which is as precise as I’d personally like to read.

I second that.

florianbrinkmann commented 2 years ago

We forgot to make the aria-live strings translatable, so marked it as draft

pfefferle commented 2 years ago

The flush-button looks like a link, so you should change the cursor, to be consistent.

florianbrinkmann commented 2 years ago

@pfefferle done!

florianbrinkmann commented 2 years ago

@angelocali and I just tested the PR with WP 4.4 to check if the REST functionality works there, and it does. The only problem was that the success dashicon we use was introduced with WP 5.2, so we added a fallback for older versions in the last commits. Would you re-review that again, @pfefferle and @bueltge? Thanks! 😇