|
Description
Agustín Moyano
2023-03-08 15:03:48 UTC
Created attachment 147962 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition 1. You forgot the translatability ;)
2. There are 2 warnings in the console
Vue received a Component which was made a reactive object. This can lead to unnecessary performance overhead, and should be avoided by marking the component with `markRaw` or using `shallowRef` instead of `ref`.
3. The last item does not have the "current" and so is not styled correctly (green instead of grey)
4. The disabled ("Local" for instance) is also styled differently
5. This will conflict with bug 32932
6. vue/stores/navigation.js should be tidied :)
(In reply to Jonathan Druart from comment #2) > 1. You forgot the translatability ;) > > 2. There are 2 warnings in the console > > Vue received a Component which was made a reactive object. This can lead to > unnecessary performance overhead, and should be avoided by marking the > component with `markRaw` or using `shallowRef` instead of `ref`. > > 3. The last item does not have the "current" and so is not styled correctly > (green instead of grey) > > 4. The disabled ("Local" for instance) is also styled differently > > 5. This will conflict with bug 32932 > > 6. vue/stores/navigation.js should be tidied :) Thank you Jonathan for the feedback! Created attachment 148044 [details] [review] Bug 33169: (follow-up) Cleanup and tidy a little bit Created attachment 148081 [details] [review] Bug 33169: (follow-up) Cleanup and tidy a little bit Created attachment 148097 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition This no longer applies after 32932 has been pushed to master. Created attachment 149710 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Changed assignee 8-). Created attachment 149744 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> There is something broken. Uncaught (in promise) Error: Missing required param "license_id" To recreate: 1. Create a license 2. Go to /cgi-bin/koha/erm/licenses 3. Click on a license's name Created attachment 151530 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> (In reply to Jonathan Druart from comment #11) > There is something broken. > > Uncaught (in promise) Error: Missing required param "license_id" > > To recreate: > 1. Create a license > 2. Go to /cgi-bin/koha/erm/licenses > 3. Click on a license's name Could no reproduce this bug... could you please try again? (In reply to Agustín Moyano from comment #13) > (In reply to Jonathan Druart from comment #11) > > There is something broken. > > > > Uncaught (in promise) Error: Missing required param "license_id" > > > > To recreate: > > 1. Create a license > > 2. Go to /cgi-bin/koha/erm/licenses > > 3. Click on a license's name > > Could no reproduce this bug... could you please try again? Tried again and still the problem. yes|git bz apply 33169 yarn js:build Create a license Go to /cgi-bin/koha/erm/licenses Open the console Click on a license's name Uncaught (in promise) Error: Missing required param "license_id" Created attachment 151797 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Hi Jonathan, please try once more. Thanks Created attachment 152068 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Not changing the status as I aint no Vue expert. But I tested things work and no warnings/errors in the console. The only thing I don't like but is not this patch responsibility, is the fact we display 'Show Thing' in the breadcrumb... I think we should be more consistent (we are not 100% consistent on this, I know but...). Right now we display Home > E-resource management > Licenses > Show license it should be: Home > E-resource management > Licenses > A licence name like we have in other places e.g Home > Tools > Patron lists > A list name I mention this in case it fits this change. (In reply to Tomás Cohen Arazi from comment #18) > Not changing the status as I aint no Vue expert. But I tested things work > and no warnings/errors in the console. > > The only thing I don't like but is not this patch responsibility, is the > fact we display 'Show Thing' in the breadcrumb... I think we should be more > consistent (we are not 100% consistent on this, I know but...). > > Right now we display > > Home > E-resource management > Licenses > Show license > > it should be: > > Home > E-resource management > Licenses > A licence name > > like we have in other places e.g > > Home > Tools > Patron lists > A list name > > I mention this in case it fits this change. It is something I didn't manage to do with the current code (I thought there was actually a FIXME in the code but there is none). Created attachment 152539 [details] [review] Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 152540 [details] [review] Bug 33169: Add support for FA styles Font awesome now provides a different approach to icon styles so you can specify things like: ``` <i class="fa fa-camera-retro"></i> ``` but also ``` <i class="fa-solid fa-camera-retro"></i> ``` The original implementation had a hardcoded `fa` and having an extra parameter for the *style* seemed overkill, as we would need to handle even a list of styles like on this example: ``` <i class="fa-sharp fa-solid fa-camera-retro"></i> ``` So I chose to just explicitly require the full FA icon class and pass it thru. With no manipulation on the Vue side. This patch does that, and adjusts the 'style' for some icons, as introduced by the FA v6 patchset. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 152541 [details] [review] Bug 33169: (QA follow-up) Tidy Vue-related files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #22) > Created attachment 152541 [details] [review] [review] > Bug 33169: (QA follow-up) Tidy Vue-related files > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thanks Tom for the follow-ups, I'll mark this as PQA (In reply to Jonathan Druart from comment #19) > (In reply to Tomás Cohen Arazi from comment #18) > > Not changing the status as I aint no Vue expert. But I tested things work > > and no warnings/errors in the console. > > > > The only thing I don't like but is not this patch responsibility, is the > > fact we display 'Show Thing' in the breadcrumb... I think we should be more > > consistent (we are not 100% consistent on this, I know but...). > > > > Right now we display > > > > Home > E-resource management > Licenses > Show license > > > > it should be: > > > > Home > E-resource management > Licenses > A licence name > > > > like we have in other places e.g > > > > Home > Tools > Patron lists > A list name > > > > I mention this in case it fits this change. > > It is something I didn't manage to do with the current code (I thought there > was actually a FIXME in the code but there is none). Maybe later I can add a method to the navigation store to be able to update the last part of the breadcrumb I'll create a new bug for it. Pushed to master for 23.11. Nice work everyone, thanks! Independent QA missing here... 1. Existing components/Breadcrumb.vue has not been removed. But please don't remove until the next point is not fixed. 2. I am trying to adjust the code from bug 30708 to use this, but I am struggling. I don't know if it's me or this implementation that is not flexible enough. I am trying to have, in the breadcrumbs: /preservation/trains/3 => Preservation > Trains > Show train /preservation/trains/3/items/add => Preservation > Trains > Add item to train Ideally we will need to display the name of the current resource, but that's another topic. I don't think it's feasible for now (discussed already in comment 18 and comment 19). With my try on https://gitlab.com/joubu/Koha/-/commits/preservation_module-wip-33169 I am getting: /preservation/trains/3 => Preservation > Trains > Show train (OK) /preservation/trains/3/items/add => Preservation > Trains > Show train > > Add item to train (KO there are 2 '>') Additionally "Show train" is /preservation/trains/:train_id Not sure this is related to this patch, but I am getting weird warning that seems coming from here [Vue Router warn]: Discarded invalid param(s) "license_id" when navigating. See https://github.com/vuejs/router/blob/main/packages/router/CHANGELOG.md#414-2022-08-22 for more details. It happens, inconsistently, when going to a FormAdd view (License or Agreement) http://kohadev-intra.mydnsname.org:8081/cgi-bin/koha/erm/licenses/add When creating a new agreement the error is about agreement_id ofc. I think this should be backported once the above issues are resolved.. it's likely to be needed for subsequent backports around erm and other vue based modules. Ping? The problems highlighted in the previous comments are blocker for future Vue developments (already in testing stage!). From IRC Joubu> tcohen[m]: any news from AgustinMoyano[m] about bug 33169? huginn> ^C04Bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33169 enhancement, P5 - low, ---, agustinmoyano, Pushed to master , Improve vue breadcrumbs and left-hand menu^C tcohen[m]> I wouldn't count on him taking on that tcohen[m]> ashimema asked me, and I told him I haven't heard from him ashimema> It's bad we now how two ways of doing almost the same thing tcohen[m]> ashimema: we have two ways because the previous wasn't removed, but it is unused, right? tcohen[m]> the missing piece here would be addressing Joubu's problem reports, right? Joubu> tcohen[m]: then who? If nobody we should revert then... Joubu> we complexified the code bringing less flexibility... tcohen[m]> it didn't look complex to me, and had Matt's signature ashimema> I'm not actually sure tcohen[m]> I can revert, but we need to make sure this is not what we are aiming for ashimema> I think both are used ashimema> maybe I'm wrong tcohen[m]> the patch replaced the 'old ones' tcohen[m]> cd koha-tmpl/intranet-tmpl/prog/js/vue tcohen[m]> git grep Breadcrumb tcohen[m]> I'd like to hear from Matthew Blenkinsop. tcohen[m]> he saw some value on this on the other bug tcohen[m]> I'm not attached to it Joubu> At least I would like to know if what I want to do is possible or if we need more code Joubu> I've tried several things but didn't manage with the current code, and I don't understand the code to fix it tcohen[m]> so you tried but didn't manage to fix it tcohen[m]> ok Joubu> if nobody understand it, apart from AgustinMoyano[m] that is not going to spend more time on it, then it's hard to keep... Joubu> ofc I've tried tcohen[m]> if nobody understands it tcohen[m]> it doesn't matter if he fixes it, right? Joubu> QA step is there to make sure not only 1 person understand the code, right? :D tcohen[m]> the code in both is almost identical tcohen[m]> well no tcohen[m]> there's the 'NavigationItem' abstraction Joubu> there is almost nothing identical between the 2 approaches Joubu> in "mine" there is a build_breadcrumb function for each item Joubu> in the "new" one, there are some flag defined in the route definition (is_navigation_item, is_end_node, etc.) then the logic is in navigation.js Joubu> store/navigation.js Joubu> that is where the logic is* I really would like to see this problem fixed, and it is preferable to fix this new implementation as it is better in the long term (less code to add and maintain in router files). However if nobody is able to provide a fix then we have a problem: we have code that we don't have under control. Or, maybe we don't even need a fix, and the current code already allows it? But how then? Created attachment 153889 [details] [review] Bug 33169: Allow breadcrumbs to accept dynamic values This patch is an example of allowing a breadcrumb to accept a dynamic value such as an agreement or license name. Created attachment 153890 [details] [review] Bug 33169: DO NOT PUSH Allow breadcrumbs to accept dynamic values This patch is an example of allowing a breadcrumb to accept a dynamic value such as an agreement or license name. The patch above is an example of an implementation for this by passing a parameter into the breadcrumbs from a particular component. Currently I've only set it up for Agreements and Licenses for discussion but the implementation is simple to repeat. I've also included two dummy routes to tackle Joubu's issue in comment 27 that would need removing once this example is reviewed. These have been added into Agreements as a substitute for the Preservation module Test plan 1) Apply and yarn js:build 2) Create an agreement 3) Click on the link to view the agreement 4) The breadcrumb should contain your agreement's name 5) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/ 6) Breadcrumb should read "Agreements" > "Your agreement name" > "Add item to train" 7) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/1 8) Breadcrumb should read "Agreements" > "Your agreement name" > "Add items to train" rel_23_05_candidate means we want backport on 23.05.x ? Patches does not apply easy because of Bug 32910 not in 23.05 Isn't it just a case of skipping the 'Add support for FA styles' patch? (In reply to Matt Blenkinsop from comment #35) > The patch above is an example of an implementation for this by passing a > parameter into the breadcrumbs from a particular component. > > Currently I've only set it up for Agreements and Licenses for discussion but > the implementation is simple to repeat. > > I've also included two dummy routes to tackle Joubu's issue in comment 27 > that would need removing once this example is reviewed. These have been > added into Agreements as a substitute for the Preservation module > > Test plan > 1) Apply and yarn js:build > 2) Create an agreement > 3) Click on the link to view the agreement > 4) The breadcrumb should contain your agreement's name > 5) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/ > 6) Breadcrumb should read "Agreements" > "Your agreement name" > "Add item > to train" > 7) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/1 > 8) Breadcrumb should read "Agreements" > "Your agreement name" > "Add items > to train" Thanks a lot Matt, I have opened 2 bug reports, I will split your patch. See bug 34417 and 34418. See you there ;) (In reply to Jonathan Druart from comment #38) > (In reply to Matt Blenkinsop from comment #35) > > The patch above is an example of an implementation for this by passing a > > parameter into the breadcrumbs from a particular component. > > > > Currently I've only set it up for Agreements and Licenses for discussion but > > the implementation is simple to repeat. > > > > I've also included two dummy routes to tackle Joubu's issue in comment 27 > > that would need removing once this example is reviewed. These have been > > added into Agreements as a substitute for the Preservation module > > > > Test plan > > 1) Apply and yarn js:build > > 2) Create an agreement > > 3) Click on the link to view the agreement > > 4) The breadcrumb should contain your agreement's name > > 5) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/ > > 6) Breadcrumb should read "Agreements" > "Your agreement name" > "Add item > > to train" > > 7) Navigate to /cgi-bin/koha/erm/agreements/1/items/add/1 > > 8) Breadcrumb should read "Agreements" > "Your agreement name" > "Add items > > to train" > > Thanks a lot Matt, I have opened 2 bug reports, I will split your patch. See > bug 34417 and 34418. See you there ;) I did not see Matt's reply before... I uploaded a patch to fix this problem in bug 34425 Could you and Matt check it out, please? Thanks (In reply to Fridolin Somers from comment #36) > rel_23_05_candidate means we want backport on 23.05.x ? > Patches does not apply easy because of Bug 32910 not in 23.05 Any update here? Created attachment 157785 [details] [review] [23.05] Bug 33169: (QA follow-up) Fix some icons for lower FA version In the navigation some icons were missing, because they had changed names between versions, this restores them. Fixed icons concerned the navigation items: * Local * Local > Titles * EBSCO > Titles Created attachment 157806 [details] [review] [23.05 without DNP-Patch] Bug 33169: (QA follow-up) Fix some icons for lower FA version In the navigation some icons were missing, because they had changed names between versions, this restores them. Fixed icons concerned the navigation items: * Local * Local > Titles * EBSCO > Titles Created attachment 157807 [details] [review] [23.05 without DNP-Patch] Bug 33169: (QA follow-up) Fix some icons for lower FA version In the navigation some icons were missing, because they had changed names between versions, this restores them. Fixed icons concerned the navigation items: * Local * Local > Titles * EBSCO > Titles The follow-up did not apply if the DO NOT PUSH patch was not included. I made a patch that works on top of a branch without that patch too. I think I get it. I've picked from branch https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05 : 7202d42b2d Bug 33169: (QA follow-up) Tidy Vue-related files 6079d70e2d Bug 33169: Add support for FA styles 31f48a6d19 Bug 33169: Build breadcrumbs and left-hand side menu from routes definition Plus the last patch attached here : 102b21d2ae Bug 33169: (QA follow-up) Fix some icons for lower FA version Looks good with Bug 33606 Pushed to 23.05.x for 23.05.05 (In reply to Fridolin Somers from comment #45) > I think I get it. > > I've picked from branch > https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05 : > > 7202d42b2d Bug 33169: (QA follow-up) Tidy Vue-related files > 6079d70e2d Bug 33169: Add support for FA styles > 31f48a6d19 Bug 33169: Build breadcrumbs and left-hand side menu from routes > definition > > Plus the last patch attached here : > > 102b21d2ae Bug 33169: (QA follow-up) Fix some icons for lower FA version > > Looks good with Bug 33606 > > Pushed to 23.05.x for 23.05.05 Hi Frido, please pick the last patch from here too - it fixes 2 broken icons in the ERM navigation (3 menu items appear without an icon, if you have all features active) (In reply to Katrin Fischer from comment #46) > (In reply to Fridolin Somers from comment #45) > > I think I get it. > > > > I've picked from branch > > https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05 : > > > > 7202d42b2d Bug 33169: (QA follow-up) Tidy Vue-related files > > 6079d70e2d Bug 33169: Add support for FA styles > > 31f48a6d19 Bug 33169: Build breadcrumbs and left-hand side menu from routes > > definition > > > > Plus the last patch attached here : > > > > 102b21d2ae Bug 33169: (QA follow-up) Fix some icons for lower FA version > > > > Looks good with Bug 33606 > > > > Pushed to 23.05.x for 23.05.05 > > Hi Frido, please pick the last patch from here too - it fixes 2 broken icons > in the ERM navigation (3 menu items appear without an icon, if you have all > features active) Clearly to late for writing comment - you got them all :) Thanks! (In reply to Katrin Fischer from comment #47) > > Clearly to late for writing comment - you got them all :) Thanks! Ouf ;) Nice work everyone! Pushed to oldstable for 22.11.x |