On bug 33169 we rewrite the way we build the breadcrumb for the Vue app. It leads to a missing use case, see bug 33169 comment 27: """ 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 """
Created attachment 153902 [details] [review] Bug 34418: DO NOT PUSH - example Try the other patch with this patch applied. Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add Without the patch you get: E-resource management > Agreements > Show agreement > > Add item to train With this patch applied: E-resource management > Agreements > Show agreement > Add item to train
Created attachment 153903 [details] [review] Bug 34418: Allow empty nodes in breadcrumb's elements Coming from an adaptation of "Bug 33169: DO NOT PUSH Allow breadcrumbs to accept dynamic values"
Matt, I am wondering if we should not have a flag in the route definition instead, we already have is_base, is_navigation_item, is_end_node and is_default. Shouldn't we have another one to flag this kind of node? Also it will clearly show in the routes that this node is always empty and can permit to catch bug more easily (Seeing the 2 "> >" in case it's empty but should not).
Created attachment 153904 [details] [review] Bug 34418: Add is_empty flag
Created attachment 153905 [details] [review] Bug 34418: DO NOT PUSH - update example
Created attachment 153910 [details] [review] Bug 34418: DO NOT PUSH - example Try the other patch with this patch applied. Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add Without the patch you get: E-resource management > Agreements > Show agreement > > Add item to train With this patch applied: E-resource management > Agreements > Show agreement > Add item to train
Created attachment 153911 [details] [review] Bug 34418: Allow empty nodes in breadcrumb's elements Coming from an adaptation of "Bug 33169: DO NOT PUSH Allow breadcrumbs to accept dynamic values"
Created attachment 153912 [details] [review] Bug 34418: Add is_empty flag
Created attachment 153913 [details] [review] Bug 34418: DO NOT PUSH - update example
(In reply to Jonathan Druart from comment #3) > Matt, I am wondering if we should not have a flag in the route definition > instead, we already have is_base, is_navigation_item, is_end_node and > is_default. > > Shouldn't we have another one to flag this kind of node? Also it will > clearly show in the routes that this node is always empty and can permit to > catch bug more easily (Seeing the 2 "> >" in case it's empty but should not). This is definitely cleaner!
*** Bug 34425 has been marked as a duplicate of this bug. ***
Created attachment 153983 [details] [review] Bug 34418: Replace dynamic parameter on unnamed route node
The last patch I pushed is to fix the link when it has parameters, for example, the "/preservation/trains/:train_id" on the Show train breadcrumb
(In reply to Agustín Moyano from comment #13) > The last patch I pushed is to fix the link when it has parameters, for > example, the "/preservation/trains/:train_id" on the Show train breadcrumb I am not sure to understand what this patch is trying to fix, could you provide more detail please? Does it apply to the ERM module as well? I have tried on the wip for the preservation module, and: Preservation > Trains > Show train > Add item to train I have "Show train" with /cgi-bin/koha/preservation/trains/1 With and without this last patch. I have adjusted the remote branch preservation_module-wip-33169
(In reply to Jonathan Druart from comment #14) > (In reply to Agustín Moyano from comment #13) > > The last patch I pushed is to fix the link when it has parameters, for > > example, the "/preservation/trains/:train_id" on the Show train breadcrumb > > I am not sure to understand what this patch is trying to fix, could you > provide more detail please? > > Does it apply to the ERM module as well? > > I have tried on the wip for the preservation module, and: > > Preservation > Trains > Show train > Add item to train > I have "Show train" with /cgi-bin/koha/preservation/trains/1 > > With and without this last patch. > > I have adjusted the remote branch preservation_module-wip-33169 According to vue-router docs, "params are ignored if a path is provided, which is not the case for query, as shown in the example above. Instead, you need to provide the name of the route or manually specify the whole path with any parameter" That means that when we build the breadcrumb if in your routes tree you did not specify a name, NavigationItem will render the second choice <router-link v-else-if="item.path" :to="item.path" :class="{ disabled: item.disabled }" > Where item.path would be /preservation/trains/:train_id I did not check that branch, but if you added names to your router (as ERM has) this patch would have no effect
I see that you added 'name: "TrainsShow"' to the node, I believe that if you remove that name, you will see the difference with or without the patch
Created attachment 154095 [details] [review] Bug 34418: Replace dynamic parameter on unnamed route node Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Agustín Moyano from comment #16) > I see that you added 'name: "TrainsShow"' to the node, I believe that if you > remove that name, you will see the difference with or without the patch Ok, I see now. It's working great! For instance, when on: Preservation > Trains > Show train > Add item to train "Show train" is "preservation/trains/42" with this patch Without this patch is was "preservation/trains/:train_id".
Created attachment 154210 [details] [review] Bug 34418: DO NOT PUSH - example Try the other patch with this patch applied. Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add Without the patch you get: E-resource management > Agreements > Show agreement > > Add item to train With this patch applied: E-resource management > Agreements > Show agreement > Add item to train Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 154211 [details] [review] Bug 34418: Allow empty nodes in breadcrumb's elements Coming from an adaptation of "Bug 33169: DO NOT PUSH Allow breadcrumbs to accept dynamic values" Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 154212 [details] [review] Bug 34418: Add is_empty flag Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 154213 [details] [review] Bug 34418: DO NOT PUSH - update example Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 154214 [details] [review] Bug 34418: Replace dynamic parameter on unnamed route node Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Paul, we need you here.
(In reply to Jonathan Druart from comment #24) > Paul, we need you here. Sorry, wrong bug, it was 34417!
Created attachment 155098 [details] [review] Bug 34418: DO NOT PUSH - example Try the other patch with this patch applied. Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add Without the patch you get: E-resource management > Agreements > Show agreement > > Add item to train With this patch applied: E-resource management > Agreements > Show agreement > Add item to train Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155099 [details] [review] Bug 34418: Allow empty nodes in breadcrumb's elements Coming from an adaptation of "Bug 33169: DO NOT PUSH Allow breadcrumbs to accept dynamic values" Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155100 [details] [review] Bug 34418: Add is_empty flag Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155101 [details] [review] Bug 34418: DO NOT PUSH - update example Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155102 [details] [review] Bug 34418: Replace dynamic parameter on unnamed route node Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Depends on Bug 33169 not in 23.05.x
(In reply to Fridolin Somers from comment #32) > Depends on Bug 33169 not in 23.05.x In the end Bug 33169 is backported. But since this is minor I propose to keep it that way.