-
Notifications
You must be signed in to change notification settings - Fork 19
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
Updated settings demo page #145
Conversation
includes/class-wp-notify-demo.php
Outdated
* | ||
* Development demo files | ||
**/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something I'm planning on taking on as a larger project, but what do you think about using a namespace for this instead of prefixes?
Probably
namespace WP\Notifications\Demo;
use WP_Admin_Bar;
etc
This looks good. A few random thoughts here:
|
1 - About the first thing (giving plugins the possibility of adding fields) I had thought about it. We just add a filter here to add a row but it is a bit more complicated to add a column... it is not impossible but at the moment I did not want to create much code for it as we still have to define it well (see next point) 2 - As a great supporter of "js for everything" I would say no because it would be completely different to the standard WordPress "way", and besides I don't see the need. Anyway this is a possibility I have explored already in this repo where I made a settings panel that saves using the rest-api. I thing that could be nice for a plugin with a lot of settings (like woocommerce) but in our case could be an unnecessary overhead. 3 - 🤦the table does not provide a confirmation button because it is not a form. this could fix |
I think your suggestions make sense. Could you update this with the latest |
What?
Updates the settings page demo code
Why?
Accessibility: settings checkbox labels are not unique #110
How?
The settings table has been refactored, now it is built by a couple of loops and the ids are dynamically generated so there should be no more duplicate ones
Testing Instructions