So far the Vue files are only used by the ERM module, but some needs to be move in a parent directory to make them reusable by another module.
Created attachment 146030 [details] [review] Bug 32806: Move Vue files for reusability
Created attachment 146033 [details] [review] Bug 32806: Rename main.js to erm.js
Created attachment 146035 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM"
Created attachment 146036 [details] [review] Bug 32806: Move routes.js to routes/erm.js
Hi Jonathan, to reuse stuff I like your idea to create folders by type (routes, stores, components, ..). Maybe we can add another folder named "modules" where we place all "main.ts" files, renaming them as "erm.ts" or "import_sources.ts". Or, instead of creating a "modules" folder, we could have a single "main.ts" and make all modules in routes to lazy load ( https://router.vuejs.org/guide/advanced/lazy-loading.html ) About the components folder, in my implementation, I added a "shared" subfolder for all shared components, but I don't mind keeping them in the components folder directly. I loved the idea of generating the breadcrumbs from the router specification, but the definition of it was kind of confusing, and hard to follow, so I took your idea and changed it a little bit. I added a couple more shared components, one of them being the implementation of Datatables in Vue (called KohaTables). I see that you have here the "composable" folder where you call functions of Datatables, but I saw that the creators of Datatables had an implementation for Vue, so I thought it best to use that. Thanks!
(In reply to Agustín Moyano from comment #5) > Hi Jonathan, to reuse stuff I like your idea to create folders by type > (routes, stores, components, ..). > > Maybe we can add another folder named "modules" where we place all "main.ts" > files, renaming them as "erm.ts" or "import_sources.ts". Or, instead of > creating a "modules" folder, we could have a single "main.ts" and make all > modules in routes to lazy load ( > https://router.vuejs.org/guide/advanced/lazy-loading.html ) I think I quite like the idea to have them visible at the root instead of hiding in the sub directory, but I am not strongly attached to it. > About the components folder, in my implementation, I added a "shared" > subfolder for all shared components, but I don't mind keeping them in the > components folder directly. Same as above, it was to make them more visible, but that one could consider it messy. > I loved the idea of generating the breadcrumbs from the router > specification, but the definition of it was kind of confusing, and hard to > follow, so I took your idea and changed it a little bit. The breadcrumb for the ERM module is quite complex, hence the complexity of the file :) About your implementation, there is something weird, you don't define the routes, and "Add import source" leads to admin/import-sources.pl/add. Is that expected? On erm I've decided to rewrite the url and have something following the REST routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance) > I added a couple more shared components, one of them being the > implementation of Datatables in Vue (called KohaTables). I see that you have > here the "composable" folder where you call functions of Datatables, but I > saw that the creators of Datatables had an implementation for Vue, so I > thought it best to use that. Yes but then you will need to reimplement all what we have in datatables.js, and I don't think that's something worthwhile (at least I would not consider it a priority for now). In your implementation there is problem with the pagination (NaN), the order by does not work, there is no column filtering, etc. I think we need to be consistent between the modules that are going to use Vue. We should apply the changes to the existing modules, then use them for the new ones.
Putting in discussion, but we need to move forward quickly here.
This looks really good. Had a look at Agustin's implementation, also looks really good. For discussion sake, here: https://gitlab.com/thekesolutions/Koha/-/tree/import_source_vue/koha-tmpl/intranet-tmpl/prog/js/vue I agree with everything and don't have much to add really, some thoughts at a glance: - Personally, I like Agustin's "modules" folder idea. - Not a huge fan of "fetch" folder, mostly the name, but don't have a great alternative for it either. - What about t/cypress? Should we prefix all module related tests with modulename and leave all tests inside integration folder where they currently live?
Created attachment 146337 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js
Created attachment 146357 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts
(In reply to Jonathan Druart from comment #6) > > About the components folder, in my implementation, I added a "shared" > > subfolder for all shared components, but I don't mind keeping them in the > > components folder directly. > > Same as above, it was to make them more visible, but that one could consider > it messy. I believe we can keep them at components folder > > > I loved the idea of generating the breadcrumbs from the router > > specification, but the definition of it was kind of confusing, and hard to > > follow, so I took your idea and changed it a little bit. > > The breadcrumb for the ERM module is quite complex, hence the complexity of > the file :) > About your implementation, there is something weird, you don't define the > routes, and "Add import source" leads to admin/import-sources.pl/add. Is > that expected? > On erm I've decided to rewrite the url and have something following the REST > routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance) > I defined my routes in import-source/tree.js That definition gets imported to stores/menu.js, and when that happens the tree gets analyzed, and some attributes are added. from there, the menu store has actions that allow you to get the routes, left-hand submenu and breadcrums About the url, if I don't make admin/import-sources.pl the root of my routes, when users enter, they would see an empty page... I really don't want users to click on something to see the table.. the other routes depend on this one, so the /add and /:id gets appended > > I added a couple more shared components, one of them being the > > implementation of Datatables in Vue (called KohaTables). I see that you have > > here the "composable" folder where you call functions of Datatables, but I > > saw that the creators of Datatables had an implementation for Vue, so I > > thought it best to use that. > > Yes but then you will need to reimplement all what we have in datatables.js, > and I don't think that's something worthwhile (at least I would not consider > it a priority for now). In your implementation there is problem with the > pagination (NaN), the order by does not work, there is no column filtering, > etc. > Ok, but I still believe all logic of Datatables should be encapsulated by a single component... meaning that what it is in composables/datatables.js should be logic that belongs to a table component (Datatable, Kohatable, or whatever you believe best to call it). In that case, if somewhere in the future we want to replace Datatables with something else, there is a single place to touch and is more consistent with Vue's "spirit" of things > I think we need to be consistent between the modules that are going to use > Vue. We should apply the changes to the existing modules, then use them for > the new ones. Agreed, I made my bug depend on this one, just to be in sync Thanks
(In reply to Agustín Moyano from comment #11) > (In reply to Jonathan Druart from comment #6) > > The breadcrumb for the ERM module is quite complex, hence the complexity of > > the file :) > > About your implementation, there is something weird, you don't define the > > routes, and "Add import source" leads to admin/import-sources.pl/add. Is > > that expected? > > On erm I've decided to rewrite the url and have something following the REST > > routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance) > > I defined my routes in import-source/tree.js > > That definition gets imported to stores/menu.js, and when that happens the > tree gets analyzed, and some attributes are added. > > from there, the menu store has actions that allow you to get the routes, > left-hand submenu and breadcrums > > About the url, if I don't make admin/import-sources.pl the root of my > routes, when users enter, they would see an empty page... I really don't > want users to click on something to see the table.. the other routes depend > on this one, so the /add and /:id gets appended You need the RewriteRule in apache's config. Another thing is the translations, did you try if that works with your implementation? I don't mind replacing the breadcrumbs system we have for ERM, but to be honest I spent a lot of time to reach this point, and my I am not sure what you have now is ready to replace it. Feel free to provide an implementation for the ERM module to see if there is feature parity. https://snipboard.io/9f6qPL.jpg
Created attachment 146465 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js
Created attachment 146466 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts
Can we move forward here?
Created attachment 146619 [details] [review] Bug 32806: Rename main.js to erm.js
Created attachment 146620 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM"
Created attachment 146621 [details] [review] Bug 32806: Move routes.js to routes/erm.js
Created attachment 146622 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js
Created attachment 146623 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de>
Created attachment 146626 [details] [review] Bug 32806: Move Vue files for reusability Forgot one..
Created attachment 146627 [details] [review] Bug 32806: Rename main.js to erm.js
Created attachment 146628 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM"
Created attachment 146629 [details] [review] Bug 32806: Move routes.js to routes/erm.js
Created attachment 146630 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js
Created attachment 146631 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de>
Created attachment 146634 [details] [review] Bug 32806: Move Vue files for reusability
Created attachment 146635 [details] [review] Bug 32806: Rename main.js to erm.js
Created attachment 146636 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM"
Created attachment 146637 [details] [review] Bug 32806: Move routes.js to routes/erm.js
Created attachment 146638 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js
Created attachment 146639 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Hi Paul. I think you may need to change the bug status to Signed Off - it looks like you have signed it off to me. Also, normally the signed off line is in all the patches. David
Created attachment 146647 [details] [review] Bug 32806: Move Vue files for reusability Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146648 [details] [review] Bug 32806: Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146649 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM" Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146650 [details] [review] Bug 32806: Move routes.js to routes/erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146651 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146652 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
I hope that I fixed it now. Thanks David!
Created attachment 146661 [details] [review] Bug 32806: Move Vue files for reusability Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146662 [details] [review] Bug 32806: Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146663 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM" Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146664 [details] [review] Bug 32806: Move routes.js to routes/erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146665 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 146666 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Patches rebased on top of bug 32898 and bug 32923.
Created attachment 147241 [details] [review] Bug 32806: Move Vue files for reusability Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147242 [details] [review] Bug 32806: Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147243 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM" Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147244 [details] [review] Bug 32806: Move routes.js to routes/erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147245 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 147246 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
(In reply to Agustín Moyano from comment #11) > Ok, but I still believe all logic of Datatables should be encapsulated by a > single component... meaning that what it is in composables/datatables.js > should be logic that belongs to a table component (Datatable, Kohatable, or > whatever you believe best to call it). In that case, if somewhere in the > future we want to replace Datatables with something else, there is a single > place to touch and is more consistent with Vue's "spirit" of things Agreed. It seems like we've got quite a bit of duplicated code in the different components. I'm not sure that we need so many? I wonder if some things are components when they could be regular Javascript objects or JSON objects. Would need to take a deeper look to know for sure...
Created attachment 147299 [details] [review] Bug 32806: Move Vue files for reusability Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147300 [details] [review] Bug 32806: Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147301 [details] [review] Bug 32806: Rename ERMHome and ERMMain to Home and Main We don't need to repeat "ERM" Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147302 [details] [review] Bug 32806: Move routes.js to routes/erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147303 [details] [review] Bug 32806: (follow-up) Rename main.js to erm.js Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 147304 [details] [review] Bug 32806: Move main-erm.ts to modules/erm.ts Looks fine. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work everyone! Pushed to stable for 22.11.x
Not needed in 22.05.x
Is this lost in this ticket or did I miss a dependency? Because I don't see any physical renaming for main.js -> erm.js and it fails to create a transitional piloting build for our production piloting library for 22.11.x (but I assume master branch too?): make[1]: *** No rule to make target 'koha-tmpl/intranet- tmpl/prog/js/vue/dist/erm.js', needed by 'pm_to_blib'. Stop. so this error means that the target is not present, and I noticed that the .../js/dist/ folder still contains main.*, but not erm.*. I did renaming additionally (this patch below), and the build was successful. --- koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js => erm.js} | 0 .../prog/js/vue/dist/{main.js.LICENSE.txt => erm.js.LICENSE.txt} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js => erm.js} (100%) rename koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js.LICENSE.txt => erm.js.LICENSE.txt} (100%) diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js b/koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js similarity index 100% rename from koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js rename to koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js.LICENSE.txt b/koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js.LICENSE.txt similarity index 100% rename from koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js.LICENSE.txt rename to koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js.LICENSE.txt --
This depends on bug 32609 which we haven't backported. Now.. I'm considering we perhaps should backport that to 22.11 for maintainability. I'll have a chat with Matt about this.. it's easy to miss these things and it falls on my bad tuition that he missed this.