search

hermawanramadhan / CodeIgniter4-DataTables

MIT License
92 stars 38 forks source link

Added support for the editor buttons #11

Closed pmagictech closed 1 year ago

pmagictech commented 2 years ago

Just added a second optional parameter to the DataTable::of. Its default value is 'id'.

hermawanramadhan commented 2 years ago

Hello. Thanks for contributing.

That's was great! I'll review and test later..

are you already test and use this code and working fine?

pmagictech commented 2 years ago

Yeah, I have actually used it on an intranet setup. You can review and test it. Let me know if you encounter any issues with it.

hermawanramadhan commented 2 years ago

sorry late response.

after I review and thinking. I can't accept this pull request, the reason :

  1. your code looks incomplete. you said: added a second optional parameter.. I didn't see this on last your pull request.. are you sure have added this? and are you sure already test this code (last pull maybe different on your local project) ?
  2. looks like your code doesn't meet for general purpose (just what you need for your project?).
  3. are your code tested for all case? minimal case for all feature for this library on documentation ? like if we use join, subquery, etc..
  4. This is my opinion. If want to make "Editor" library. maybe better if this "Editor" has Separated library or Class.
pmagictech commented 2 years ago
  1. Sorry, I tested the library on a different setup and I forgot to update the constructor in my commit. I have fixed it in my recent commit.
  2. I don't understand, I feel having the buttons working is also for general purpose as seen here in the Datatables editor homepage https://editor.datatables.net.
  3. Yes, I have tested it in all use cases like join (the project that required this update had a join in the query), subquery etc. I ensured that the update does not affect the existing functionalities of the library. So users of the library do not need to update their codebase because of the update. They only need to add the primary key parameter if they want the buttons to work (it will work without their intervention if their primary key is 'id').
  4. Based on the point above, I don't think that is necessary for now. The purpose of this was just to make the library handle the buttons without affecting the existing functionalities of the library.
pmagictech commented 1 year ago

Hello @hermawanramadhan,

I have worked issue you mentioned. Please go through my latest update and let me if you encounter any other issues with the update.