Description
Eivin Giske Skaaren
2015-05-12 17:58:05 UTC
Created attachment 39104 [details] [review] Added the functionality to most staff header search boxes Created attachment 39105 [details] [review] Bug 14189 - Make text persist/populate hidden fields for the Staff searchbox Created attachment 39106 [details] [review] Added the functionality to most staff header search boxes Created attachment 39107 [details] [review] Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. Created attachment 39108 [details] [review] Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. Sponsored-by: Halland County Library Created attachment 39110 [details] [review] Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. (More files changed for persistent text in searchbox) Sponsored-by: Halland County Library Created attachment 39112 [details] [review] Bug 14189 - [SIGNED-OFF] Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. (More files changed for persistent text in searchbox) Sponsored-by: Halland County Library http://bugs.koha-community.org/show_bug.cgi?id=14189 Signed-off-by: Magnus Enger <magnus@enger.priv.no> This is something I have wanted quite a few times over the years... Tested by going to every main area of Koha, entering some random text into the search box and then clicking on all the available tabs to check that the entered text is carried over to all the boxes. There are a couple of places where text is not carried over, but I guess that might be because one of the boxes is structurally different to the others. These are: - "Vendor search" and "Orders search" in Acquisitions - "Search subscriptions" in Serials I have not looked at how this is implemented, just that it works as it should. What Magnus discovered with the few places it is not implemented was done intentionally as it did not make sense from a UX perspective for example when there is 2 search boxes on one tab and 1 on the other. It should work for most daily use cases where this would be helpful. The templates should probably at some point be refactored since there was some duplication of HTML, but that is not needed for this patch. QA comments: 1/ The class name could be more specific to avoid further conflict ('query') 2/ Since a lot of patch will use keep_text JS function, maybe the include of searchbox-keep-text.inc could be done once in doc-head-close.inc 3/ What is the meaning of the different values of the clicked parameter (0, 1, 2)? Please add comment. 4/ Is is intended not to use jQuery selectors in the keep_text JS function? Marked as Failed QA. Created attachment 39462 [details] [review] Bug 14189 refactor after failed QA. 1) Renamed to "head-searchbox". 2) Good suggestion, I have included it once in doc-head-close.inc. 3) For the algorithm to work when there are just 2 boxes we need to know if the first tab was clicked and since this is JS and run when a href is clicked the easiest way was to send the index as a parameter. 4) Pure JS is faster than jQuery and this code will run on every pageload and I don't think jQuery would make it any clearer than the used document.getElementsByClassName("head-searchbox"). For the rest of the JS it does not rely on names since all searchboxes of the class "head-searchbox" are put into the array "searchboxes". If a new page with any number of searchboxes is added to Koha later this should still work without modification. Reactivated old patch as told on IRC by cait. I think only the last patch is needed to apply since that was the starting point for the refactor. Created attachment 39505 [details] [review] Bug 14189 - Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. (More files changed for persistent text in searchbox) Sponsored-by: Halland County Library http://bugs.koha-community.org/show_bug.cgi?id=14189 Signed-off-by: Magnus Enger <magnus@enger.priv.no> This is something I have wanted quite a few times over the years... Tested by going to every main area of Koha, entering some random text into the search box and then clicking on all the available tabs to check that the entered text is carried over to all the boxes. There are a couple of places where text is not carried over, but I guess that might be because one of the boxes is structurally different to the others. These are: - "Vendor search" and "Orders search" in Acquisitions - "Search subscriptions" in Serials I have not looked at how this is implemented, just that it works as it should. Bug 14189 refactor after failed QA. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39506 [details] [review] Bug 14189: Use keep_text for the authority header includes/authorities-search.inc already used the same behavior but with another method. For consistency it's better to have only 1 way to do that. Test plan: Go on the authority home page Fill the input in the header and click on tab 'search all headings' and 'keyword search' and confirm that the text you have filled is kept. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Elvin, Thanks for your followup. I have squashed the 2 first patches to avoid too many add/del lines in the same patch set (same lines were added in the 1st and removed in the 2nd). Could you please test my patch for the authority header? Created attachment 39543 [details] [review] Bug 14189: Use keep_text for the authority header includes/authorities-search.inc already used the same behavior but with another method. For consistency it's better to have only 1 way to do that. Test plan: Go on the authority home page Fill the input in the header and click on tab 'search all headings' and 'keyword search' and confirm that the text you have filled is kept. I tested after applying the patch and the text is kept after removing the part from authorities-search.inc. I also tried applying that jQuery solution instead of my JS loop but could not get it to work for the other pages with a reasonable ammount of effort. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Signed-off-by: Eivin Giske Skaaren <eskaaren@yahoo.no> Created attachment 39587 [details] [review] Bug 14189 - Made text persist when clicking the tabs under the searchbox To test: Apply the patch and see that the text now is there in the search box when clicking the tabs: check in, check out etc.. (More files changed for persistent text in searchbox) Sponsored-by: Halland County Library Signed-off-by: Magnus Enger <magnus@enger.priv.no> This is something I have wanted quite a few times over the years... Tested by going to every main area of Koha, entering some random text into the search box and then clicking on all the available tabs to check that the entered text is carried over to all the boxes. There are a couple of places where text is not carried over, but I guess that might be because one of the boxes is structurally different to the others. These are: - "Vendor search" and "Orders search" in Acquisitions - "Search subscriptions" in Serials I have not looked at how this is implemented, just that it works as it should. Bug 14189 refactor after failed QA. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Amended patch: replace tabs with spaces Created attachment 39588 [details] [review] Bug 14189: Use keep_text for the authority header includes/authorities-search.inc already used the same behavior but with another method. For consistency it's better to have only 1 way to do that. Test plan: Go on the authority home page Fill the input in the header and click on tab 'search all headings' and 'keyword search' and confirm that the text you have filled is kept. I tested after applying the patch and the text is kept after removing the part from authorities-search.inc. I also tried applying that jQuery solution instead of my JS loop but could not get it to work for the other pages with a reasonable ammount of effort. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Signed-off-by: Eivin Giske Skaaren <eskaaren@yahoo.no> Patches pushed to master. Thanks Eivin and Jonathan! |