Bug 33417 - Create one standard Toolbar component
Summary: Create one standard Toolbar component
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matt Blenkinsop
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 32932
Blocks: 34214 34215
  Show dependency treegraph
 
Reported: 2023-04-05 11:55 UTC by Matt Blenkinsop
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08


Attachments
Bug 33417: Create standard toolbar component (1.58 KB, patch)
2023-04-05 12:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Add toolbar imports and options (6.23 KB, patch)
2023-04-05 12:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Remove old components (4.04 KB, patch)
2023-04-05 12:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Fix translation in toolbar (1.05 KB, patch)
2023-04-05 14:03 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Fix translation in toolbar (1.06 KB, patch)
2023-04-05 14:54 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Translation example (164.01 KB, image/png)
2023-04-05 14:54 UTC, Matt Blenkinsop
Details
Bug 33417: Fix translation in toolbar (4.13 KB, patch)
2023-04-06 11:14 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Create standard toolbar component (1.58 KB, patch)
2023-04-20 12:28 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Add toolbar imports and options (6.16 KB, patch)
2023-04-20 12:28 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Remove old components (4.04 KB, patch)
2023-04-20 12:28 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Fix translation in toolbar (4.17 KB, patch)
2023-04-20 12:28 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33417: Create standard toolbar component (1.62 KB, patch)
2023-04-20 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33417: Add toolbar imports and options (6.21 KB, patch)
2023-04-20 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33417: Remove old components (4.09 KB, patch)
2023-04-20 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33417: Fix translation in toolbar (4.18 KB, patch)
2023-04-20 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33417: Create standard toolbar component (1.70 KB, patch)
2023-06-09 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33417: Add toolbar imports and options (6.29 KB, patch)
2023-06-09 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33417: Remove old components (4.17 KB, patch)
2023-06-09 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33417: Fix translation in toolbar (4.27 KB, patch)
2023-06-09 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2023-04-05 11:55:14 UTC
We currently have multiple Toolbar components with duplicate code - this could be standardised into one reusable component
Comment 1 Matt Blenkinsop 2023-04-05 12:12:15 UTC
Created attachment 149193 [details] [review]
Bug 33417: Create standard toolbar component

This commit introduces a standard Toolbar component that can be used
throughout ERM

Test plan:
1) Apply patch
2) Navigate to Agreements and click the "Add agreement" button - this
   should work as normal
3) Navigate to Licenses and repeat
4) Navigate to Local packages and repeat
5) Navigate to local titles - there should be two buttons, one to add and
   one to import. These should work as normal
Comment 2 Matt Blenkinsop 2023-04-05 12:12:17 UTC
Created attachment 149194 [details] [review]
Bug 33417: Add toolbar imports and options

This commit amends the required files to import the toolbar and add the
correct options

Test plan as above
Comment 3 Matt Blenkinsop 2023-04-05 12:12:19 UTC
Created attachment 149195 [details] [review]
Bug 33417: Remove old components

This commit deletes the old toolbar components as they are no longer
necessary

Test plan as above
Comment 4 Matt Blenkinsop 2023-04-05 12:13:05 UTC
Not ready for signoff yet as 32932 needs a rebase and some fixes
Comment 5 Jonathan Druart 2023-04-05 12:35:05 UTC
+            {{ $__(`${option.button_title}`) }}</router-link

This won't work, it won't be picked by the translate tool.
You need to explicitly write $__("Your string") in the source code.
Comment 6 Jonathan Druart 2023-04-05 12:37:30 UTC
For a Vue module I am writing I need to show/hide button from the toolbar depending on other states. Wondering how easy/hard it will be with this new implementation.
Comment 7 Matt Blenkinsop 2023-04-05 13:44:31 UTC
(In reply to Jonathan Druart from comment #6)
> For a Vue module I am writing I need to show/hide button from the toolbar
> depending on other states. Wondering how easy/hard it will be with this new
> implementation.

My first thoughts would be that filtering the toolbar_options array would show/hide buttons, perhaps a toolbar_options and a filtered_toolbar_options would be necessary
Comment 8 Matt Blenkinsop 2023-04-05 14:03:59 UTC
Created attachment 149212 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated using
String.prototype.format()

Test plan as above
Comment 9 Jonathan Druart 2023-04-05 14:17:34 UTC
(In reply to Matt Blenkinsop from comment #8)
> Created attachment 149212 [details] [review] [review]
> Bug 33417: Fix translation in toolbar
> 
> This commit allows the string to be translated using
> String.prototype.format()
> 
> Test plan as above

Have you tried it? I haven't :)
But I don't think it will work.

Try that:
Replace "New agreement" with something that is NOT in the PO already, then run the translate script
=> Make sure it's in the PO

I am pretty sure it does not work and that you need to explicitly have $__("New agreement") in the code base.
Comment 10 Matt Blenkinsop 2023-04-05 14:54:10 UTC
Created attachment 149215 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated - tested using fr-FR and a dummy string in the .po file (see image attached)

Test plan as above
Comment 11 Matt Blenkinsop 2023-04-05 14:54:42 UTC
Created attachment 149217 [details]
Translation example
Comment 12 Jonathan Druart 2023-04-06 07:02:52 UTC
Tested, and it does not work.

% git --no-pager diff **/*.vue
diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsList.vue b/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsList.vue
index 5fee4862f17..a2bca463341 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsList.vue
+++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsList.vue
@@ -88,10 +88,12 @@ export default {
             toolbar_options: [
                 {
                     to: "AgreementsFormAdd",
-                    button_title: "New agreement",
+                    button_title: "Here is a test string",
                 },
             ],
+            another_string: this.$__("Here is ANOTHER string"),
         }
     },
     beforeRouteEnter(to, from, next) {
         next(vm => {


% cd misc/translator
% perl translate update fr-FR
% git grep 'Here is .* string' misc/translator/po/fr-FR-*
misc/translator/po/fr-FR-messages-js.po:msgid "Here is ANOTHER string"

Matt, you are not supposed to add the string in the po file manually. What I meant previously is that the 'update' part of the translate process is picking the strings from the source code (tt, inc, js and vue files).

To make it aware of the strings (and so new strings) you MUST use explicitly the __() function ($__() in vue files)

If you didn't add the string manually in the po file then tell me which black magic you used ;)
Comment 13 Matt Blenkinsop 2023-04-06 08:18:42 UTC
Sorry, translations are new to me! I'll try again
Comment 14 Matt Blenkinsop 2023-04-06 11:14:54 UTC
Created attachment 149229 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated - tested using fr-FR and a dummy string in the .po file

Test plan as above
Comment 15 Matt Blenkinsop 2023-04-20 12:28:08 UTC
Created attachment 149936 [details] [review]
Bug 33417: Create standard toolbar component

This commit introduces a standard Toolbar component that can be used
throughout ERM

Test plan:
1) Apply patch
2) Navigate to Agreements and click the "Add agreement" button - this
   should work as normal
3) Navigate to Licenses and repeat
4) Navigate to Local packages and repeat
5) Navigate to local titles - there should be two buttons, one to add and
   one to import. These should work as normal
Comment 16 Matt Blenkinsop 2023-04-20 12:28:11 UTC
Created attachment 149937 [details] [review]
Bug 33417: Add toolbar imports and options

This commit amends the required files to import the toolbar and add the
correct options

Test plan as above
Comment 17 Matt Blenkinsop 2023-04-20 12:28:13 UTC
Created attachment 149938 [details] [review]
Bug 33417: Remove old components

This commit deletes the old toolbar components as they are no longer
necessary

Test plan as above
Comment 18 Matt Blenkinsop 2023-04-20 12:28:15 UTC
Created attachment 149939 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated - tested using fr-FR and a dummy string in the .po file

Test plan as above
Comment 19 Martin Renvoize 2023-04-20 13:31:06 UTC
Created attachment 149964 [details] [review]
Bug 33417: Create standard toolbar component

This commit introduces a standard Toolbar component that can be used
throughout ERM

Test plan:
1) Apply patch
2) Navigate to Agreements and click the "Add agreement" button - this
   should work as normal
3) Navigate to Licenses and repeat
4) Navigate to Local packages and repeat
5) Navigate to local titles - there should be two buttons, one to add and
   one to import. These should work as normal

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-04-20 13:31:08 UTC
Created attachment 149965 [details] [review]
Bug 33417: Add toolbar imports and options

This commit amends the required files to import the toolbar and add the
correct options

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2023-04-20 13:31:11 UTC
Created attachment 149966 [details] [review]
Bug 33417: Remove old components

This commit deletes the old toolbar components as they are no longer
necessary

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-04-20 13:31:14 UTC
Created attachment 149967 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated - tested using fr-FR and a dummy string in the .po file

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2023-04-20 13:31:44 UTC
All working as expected and the translations working nicely now.. well spotted Jonathan.

I'm happy to signoff.
Comment 24 Martin Renvoize 2023-04-27 11:22:42 UTC
With Jonathan highlighting the translation points here I think we can call his comments a signoff.. my testing confirms all is working and I'm not seeing any regressions.. so I'm tempted to convert this to PQA.
Comment 25 Jonathan Druart 2023-04-27 12:50:48 UTC
I am not sure, see comment 6. I would like to give it a try, but I am lacking time at the moment...
Comment 26 Jonathan Druart 2023-06-09 13:12:46 UTC
Created attachment 152244 [details] [review]
Bug 33417: Create standard toolbar component

This commit introduces a standard Toolbar component that can be used
throughout ERM

Test plan:
1) Apply patch
2) Navigate to Agreements and click the "Add agreement" button - this
   should work as normal
3) Navigate to Licenses and repeat
4) Navigate to Local packages and repeat
5) Navigate to local titles - there should be two buttons, one to add and
   one to import. These should work as normal

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2023-06-09 13:12:49 UTC
Created attachment 152245 [details] [review]
Bug 33417: Add toolbar imports and options

This commit amends the required files to import the toolbar and add the
correct options

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2023-06-09 13:12:52 UTC
Created attachment 152246 [details] [review]
Bug 33417: Remove old components

This commit deletes the old toolbar components as they are no longer
necessary

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2023-06-09 13:12:55 UTC
Created attachment 152247 [details] [review]
Bug 33417: Fix translation in toolbar

This commit allows the string to be translated - tested using fr-FR and a dummy string in the .po file

Test plan as above

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2023-06-09 13:13:07 UTC
Sorry for the delay!
Comment 31 Tomás Cohen Arazi 2023-06-14 12:18:24 UTC
(In reply to Jonathan Druart from comment #30)
> Sorry for the delay!

Thanks for reviewing, Jonathan!
Comment 32 Tomás Cohen Arazi 2023-06-14 12:24:29 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 33 Jonathan Druart 2023-07-06 07:18:05 UTC
This is adding the 'plus' icon without making it configurable. Please see bug 34214.
Comment 34 Jonathan Druart 2023-07-06 08:23:06 UTC
This is not flexible enough for the needs I have on bug 30708. I have attached an alternative version on bug 34215.
Comment 35 Martin Renvoize 2023-07-17 11:22:51 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 36 Pedro Amorim 2023-07-18 10:50:12 UTC
Nice work everyone!

Pushed to 22.11.x for next release