| Summary: | The datatables api wrapper is ambiguously named | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Martin Renvoize (ashimema) <martin.renvoize> |
| Component: | Architecture, internals, and plumbing | Assignee: | Martin Renvoize (ashimema) <martin.renvoize> |
| Status: | CLOSED FIXED | QA Contact: | Kyle M Hall (khall) <kyle> |
| Severity: | trivial | ||
| Priority: | P5 - low | CC: | david, dcook, fridolin.somers, jonathan.druart, kyle, oleonard, tomascohen, victor |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29397 | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This patch 1) renames the Koha REST JS dataTables wrapper from the
ambiguous 'api' to the clearer 'kohaTable' 2) goes through the codebase and updates existing relevant calls to .api referencing the Koha REST dataTables wrapper to use the name 'kohaTable', and 3) adds JSDoc formatted parameter documentation for the kohaTable function.
|
Version(s) released in: |
21.11.00,21.05.05
|
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 29896, 28854, 29302 | ||
| Attachments: |
Bug 29408: Rename Koha dataTables wrapper
Bug 29408: Update all existing uses of .api to .kohaTable Bug 29408: Begginings of JSDoc documentation Bug 29408: Rename Koha dataTables wrapper Bug 29408: Update all existing uses of .api to .kohaTable Bug 29408: Begginings of JSDoc documentation Bug 29408: Rename Koha dataTables wrapper Bug 29408: Update all existing uses of .api to .kohaTable Bug 29408: Beginnings of JSDoc documentation Bug 29408: Rename Koha dataTables wrapper Bug 29408: Update all existing uses of .api to .kohaTable Bug 29408: Add JSDoc documentation for kohaTable function |
||
|
Description
Martin Renvoize (ashimema)
2021-11-03 16:24:20 UTC
Created attachment 127272 [details] [review] Bug 29408: Rename Koha dataTables wrapper This patch renames the Koha REST JS dataTables wrapper from the ambigious 'api' to the clearer 'kohaTable'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 127273 [details] [review] Bug 29408: Update all existing uses of .api to .kohaTable This patch goes through the codebase and updates existing revelant calls to .api referencing the Koha REST dataTables wrapper to use the name name 'kohaTable'. Test plan. 1) Ensure the tables on the following pages all continue to work as expected a) acqui/parcel b) admin/branches c) admin/cities d) admin/smtp_servers e) pos/register f) tools/quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 127274 [details] [review] Bug 29408: Begginings of JSDoc documentation I'd love to see the parameters clearly documented... I've started it off but need some help. Created attachment 127282 [details] [review] Bug 29408: Rename Koha dataTables wrapper This patch renames the Koha REST JS dataTables wrapper from the ambigious 'api' to the clearer 'kohaTable'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 127283 [details] [review] Bug 29408: Update all existing uses of .api to .kohaTable This patch goes through the codebase and updates existing revelant calls to .api referencing the Koha REST dataTables wrapper to use the name name 'kohaTable'. Test plan. 1) Ensure the tables on the following pages all continue to work as expected a) acqui/parcel b) admin/branches c) admin/cities d) admin/smtp_servers e) pos/register f) tools/quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 127284 [details] [review] Bug 29408: Begginings of JSDoc documentation I'd love to see the parameters clearly documented... I've started it off but need some help. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I did
git grep '\.api('
and the cases I found are correct (i.e. JS accessing the .api() method from DataTables.
All looks good and works as before this patchset.
Thanks for testing.. yup, those two calls to .api( that still exist are one of the reasons I was really keen to make this change :-). Created attachment 127290 [details] [review] Bug 29408: Rename Koha dataTables wrapper This patch renames the Koha REST JS dataTables wrapper from the ambigious 'api' to the clearer 'kohaTable'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 127291 [details] [review] Bug 29408: Update all existing uses of .api to .kohaTable This patch goes through the codebase and updates existing revelant calls to .api referencing the Koha REST dataTables wrapper to use the name name 'kohaTable'. Test plan. 1) Ensure the tables on the following pages all continue to work as expected a) acqui/parcel b) admin/branches c) admin/cities d) admin/smtp_servers e) pos/register f) tools/quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 127292 [details] [review] Bug 29408: Beginnings of JSDoc documentation I'd love to see the parameters clearly documented... I've started it off but need some help. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> We already have "KohaTable", isn't this more ambiguous? (In reply to Jonathan Druart from comment #12) > We already have "KohaTable", isn't this more ambiguous? I think the difference between `KohaTable('selector', settings)` and `$('selector').kohaTable(settings)` is clearer than the current `$('selector').api(settings)`.. what is `.api()`.. the jQuery API, the selected elements api.. it also doesn't help that datatables itself has an api() method to get back the datatables api object. I think we should slowly be migrating from template-driven datatables (KohaTable) to api driven datatable (kohaTable).. so to me, the similarity is deliberate and the distinction is clear. Also, by starting to add JSDoc, my hope is people will reach for kohaTable first in the future ;) I do have a couple of improvements to make on that last patch though.. I clarified a few things with Tomas this morning, but Kyle was super quick to review what was already there before I got to update it ;) Created attachment 127313 [details] [review] Bug 29408: Rename Koha dataTables wrapper This patch renames the Koha REST JS dataTables wrapper from the ambigious 'api' to the clearer 'kohaTable'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 127314 [details] [review] Bug 29408: Update all existing uses of .api to .kohaTable This patch goes through the codebase and updates existing revelant calls to .api referencing the Koha REST dataTables wrapper to use the name name 'kohaTable'. Test plan. 1) Ensure the tables on the following pages all continue to work as expected a) acqui/parcel b) admin/branches c) admin/cities d) admin/smtp_servers e) pos/register f) tools/quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 127315 [details] [review] Bug 29408: Add JSDoc documentation for kohaTable function This patch adds some JSDoc formatted parameter documentation for the kohaTable function. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Amendments to documentation made, with agreements from Tomas.. back to PQA Pushed to master for 21.11, thanks to everybody involved! Steps forward! \o/ (In reply to Martin Renvoize from comment #13) > > I think we should slowly be migrating from template-driven datatables > (KohaTable) to api driven datatable (kohaTable).. so to me, the similarity > is deliberate and the distinction is clear. > Agreed and much appreciated! The .api() method name was driving me crazy. Pushed to 21.05.x for 21.05.05 Shouldn't we also change in koha-tmpl/opac-tmpl/bootstrap/js/datatables.js ? Good call... I didn't actually realise we had the api wrapper on the opac as yet... We need a better way of keeping files like this in sync.. I'd put money on there being other bits of the wrapper that are already out of sync Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. (and if the above comments are followed upon) |