leocaseiro
commented
8 years ago Hi @lucasbhjf,
First of all, thank you so much for this amazing work. I loved the simplest of your work. I've been always thinking a way to implement that, but I've planned to do something really complicated.
However, before I merge, I have some considerations.
Be free to implement yourself, or I might code myself. Up to you. (PS: I haven't installed and tested yet, I just did a code review)
1) I don't see why we would force TRUNCATE the table. There are some cases when the user would like to import only, but keeping their old data. How about we make the TRUNCATE optional with a checkbox, something like:
- [ ] click here to clear table before import new data
I believe we need to ignore the auto-increment ID for these cases.
2) The functions name are too generic and I'm worried if it can cause a conflict with some other plugins or theme.
It's an old library(since 2008 before github) and I didn't use OOP at that time, so the best way would name functions with prefix, in that case cop which stands for "Custom Option Plus".
Eq: cop_import_data() and cop_export_data()
For the ajax actions, I normally like simulating a directory for this kind, so I would rename for:
wp_ajax_cop/import and wp_ajax_cop/export
3) I also recommend the use of the functions wp_send_json_error() and wp_send_json_success() for AJAX, instead of echo json_encode(['err' => false]); exit for example.
To clarify: They do the same job. It's just an easy way to read, in my opinion.
4) For the Javascript, I'd like to implement an IIFE or a jQuery closure. I don't see why we would use global variables such as var $ = jQuery which can also conflict with another plugin/theme.
The same way the global object importExport it's quite generic and can conflict some third-party plugin/theme.
I really like your singleton. I normally code singletons when I code jQuery. Thanks for coding that way. (Again, I coded this plugin in 2008-2011).
I would wrap the importExport object into a jQuery closure similar what I've done in functions.js, so we don't need $.noConflict() or other var importExport wouldn't affect here and vice versa:
jQuery(document).ready(function($) {
var importExport = {..}
importExport.init();
});Lucas, thanks a lot for this work. I'm sorry being a little pick, but this simple plugin has been used for over 10k users and we need to make sure they won't have any issues with the update.
É nóis!
ghost
Hello! Thanks for plugin, @leocaseiro, I'm brazilian too and i i'm wordpress developer for 1 year almost, i did some changes in plugin to support export and import and i'm about to do some mods to use it in my work, i hope you approve that, regards.