The new and previous links should be a link to the new and previous items in the list.
Hi Jonathan, I am not sure I understand the intended change? Isn't that how it works now? Showing you what's next and previous on the shelf?
Created attachment 20964 [details] [review] MT11899: Improve the previous and next items on the shelf browser The next and previous links should be a link to the next and previous items in the list. Before this patch, the previous was the first on the list and the next was the last. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. Now, the next and previous links are the same as ... 0 and 7. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links.
Created attachment 20965 [details] [review] Bug 10856: Improve the previous and next items on the shelf browser The next and previous links should be a link to the next and previous items in the list. Before this patch, the previous was the first on the list and the next was the last. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. Now, the next and previous links are the same as ... 0 and 7. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links.
(In reply to Katrin Fischer from comment #1) > Hi Jonathan, I am not sure I understand the intended change? > Isn't that how it works now? Showing you what's next and previous on the > shelf? I agree with you, the description is not clear... In fact the patch simply increments by 1 the gap.
This still doesn't work the way I expect it should. Perhaps it's not working correctly, or perhaps we have different expectations. I think that if I start from here: [<] [1] [2] [3] [4] [5] [6] [>] ...and click [>], I should see: [<] [7] [8] [9] [10] [11] [12] [>] This is the behavior one finds on, for example, Amazon.com with their "Customers Who Bought This Item Also Bought" feature. After applying this patch I go from this: [<] [1] [2] [3] [4] [5] [6] [>] ...to this: [<] [4] [5] [6] [7] [8] [9] [>] ...and that doesn't make sense to me. I don't want to push my luck, but could a solution to Bug 5500 be rolled into this one?
Created attachment 21002 [details] [review] Bug 10856: Improve the previous and next items on the shelf browser The next and previous links should completely refresh the shelf. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. With this patch, after clicking on next, we will get: [<] [7] [8] [9] [10] [11] [12] [13] [>] This patch adds a new ajax script to get the shelf browser block. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links. - Deactivate Javascript (using NoScript for example) and check that you get the same behavior (but the page is reloaded). - Launch the unit tests: prove t/db_dependent/ShelfBrowser.t
(In reply to Owen Leonard from comment #5) > This still doesn't work the way I expect it should. Perhaps it's not working > correctly, or perhaps we have different expectations. > > I think that if I start from here: > > [<] [1] [2] [3] [4] [5] [6] [>] > > ...and click [>], I should see: > > [<] [7] [8] [9] [10] [11] [12] [>] The last patch implements this behavior :)
(In reply to Owen Leonard from comment #5) > I don't want to push my luck, but could a solution to Bug 5500 be rolled > into this one? And it fixes bug 5500
*** Bug 5500 has been marked as a duplicate of this bug. ***
This is great stuff--I'm really excited about this patch! However, I'm still seeing some weird behavior from the paging buttons: With JavaScript disabled, the "next" button appears to work the same as the "previous" button. With JavaScript enabled, I think the calculations of the next and previous is not being calculated correctly. Here are some results from my test. In this example "787" represents the starting item number, the item on which the "browse shelf" link was clicked. Each row of numbers shows the item numbers in the set of results. +---------------+------+-------+-------+------+------+------+------+ | Starting set: | 8164 | 10263 | 10508 | 787 | 793 | 798 | 802 | | Next set: | 798 | 793 | 787 | 1281 | 1283 | 1284 | 1286 | | Next set: | 798 | 793 | 797 | 1318 | 1322 | 1324 | 1326 | +---------------+------+-------+-------+------+------+------+------+ For reference, here are the relevant item numbers from my database in order starting with 787: +------------+----------+----------------+ | itemnumber | cn_sort | itemcallnumber | +------------+----------+----------------+ | 787 | J_ABBOTT | J Abbott | | 793 | J_ABBOTT | J Abbott | | 798 | J_ABBOTT | J Abbott | | 802 | J_ABBOTT | J Abbott | | 803 | J_ABBOTT | J Abbott | | 1278 | J_ABBOTT | J Abbott | | 1280 | J_ABBOTT | J Abbott | | 1281 | J_ABBOTT | J Abbott | | 1283 | J_ABBOTT | J Abbott | | 1284 | J_ABBOTT | J Abbott | | 1286 | J_ABBOTT | J Abbott | | 1288 | J_ABBOTT | J Abbott | | 1289 | J_ABBOTT | J Abbott | | 1291 | J_ABBOTT | J Abbott | | 1318 | J_ABBOTT | J Abbott | | 1322 | J_ABBOTT | J Abbott | | 1324 | J_ABBOTT | J Abbott | | 1326 | J_ABBOTT | J Abbott | +------------+----------+----------------+ Looks like the right-hand side of the results is being calculated correctly but not the left hand.
Created attachment 21071 [details] [review] Bug 10856: If callnumbers are equals, order should be on itemnumbers If a lot of items has the same callnumber, the order should be on the itemnumbers. Otherwise the left side is always filled with the same items. + Fix a bad c/p for the next link (when js is disabled).
(In reply to Owen Leonard from comment #10) > This is great stuff--I'm really excited about this patch! However, I'm still > seeing some weird behavior from the paging buttons: > > With JavaScript disabled, the "next" button appears to work the same as the > "previous" button. Oops, bad c/p. Fixed! > Looks like the right-hand side of the results is being calculated correctly > but not the left hand. This already seems to exist on master. The query did not sort by itemnumbers DESC on the left side. Should be fixed.
Created attachment 21136 [details] [review] [SIGNED-OFF] Bug 10856: Improve the previous and next items on the shelf browser The next and previous links should completely refresh the shelf. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. With this patch, after clicking on next, we will get: [<] [7] [8] [9] [10] [11] [12] [13] [>] This patch adds a new ajax script to get the shelf browser block. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links. - Deactivate Javascript (using NoScript for example) and check that you get the same behavior (but the page is reloaded). - Launch the unit tests: prove t/db_dependent/ShelfBrowser.t Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 21137 [details] [review] [SIGNED-OFF] Bug 10856: If callnumbers are equals, order should be on itemnumbers If a lot of items has the same callnumber, the order should be on the itemnumbers. Otherwise the left side is always filled with the same items. + Fix a bad c/p for the next link (when js is disabled). Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 21138 [details] [review] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser In Firefox at least, the shelf browser cannot be reopened after hiding it with the "close shelf browser" link. This followup improves the behavior of the "close shelf browser" link so that the shelf browser can be redisplayed. To test, open a bibliographic detail page in the OPAC and click a "browse shelf" link. Click the "close shelf browser" link--the shelf browser should be hidden. Click the original "browse shelf" link and the shelf browser should reappear without reloading the page. Test with Firefox and Chrome (at least).
Created attachment 21211 [details] [review] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser In Firefox at least, the shelf browser cannot be reopened after hiding it with the "close shelf browser" link. This followup improves the behavior of the "close shelf browser" link so that the shelf browser can be redisplayed. To test, open a bibliographic detail page in the OPAC and click a "browse shelf" link. Click the "close shelf browser" link--the shelf browser should be hidden. Click the original "browse shelf" link and the shelf browser should reappear without reloading the page. Test with Firefox and Chrome (at least). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 21319 [details] [review] [PASSED QA] Bug 10856: Improve the previous and next items on the shelf browser The next and previous links should completely refresh the shelf. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. With this patch, after clicking on next, we will get: [<] [7] [8] [9] [10] [11] [12] [13] [>] This patch adds a new ajax script to get the shelf browser block. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links. - Deactivate Javascript (using NoScript for example) and check that you get the same behavior (but the page is reloaded). - Launch the unit tests: prove t/db_dependent/ShelfBrowser.t Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 21320 [details] [review] [PASSED QA] Bug 10856: If callnumbers are equals, order should be on itemnumbers If a lot of items has the same callnumber, the order should be on the itemnumbers. Otherwise the left side is always filled with the same items. + Fix a bad c/p for the next link (when js is disabled). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 21321 [details] [review] [PASSED QA] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser In Firefox at least, the shelf browser cannot be reopened after hiding it with the "close shelf browser" link. This followup improves the behavior of the "close shelf browser" link so that the shelf browser can be redisplayed. To test, open a bibliographic detail page in the OPAC and click a "browse shelf" link. Click the "close shelf browser" link--the shelf browser should be hidden. Click the original "browse shelf" link and the shelf browser should reappear without reloading the page. Test with Firefox and Chrome (at least). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Testing notes: - New unit tests in t/db_dependent/ShelfBrowser.t pass. - System preference OPACShelfBrowser still works as expected. - Closing and opening the shelf browser works as expected. - Next and Previous links show new and nicer behaviour. - Logs are clean. Tested with Firefox and Chromium under Ubuntu. Notes: The currently displayed record could maybe be highlighted a bit better.
This is close, but I found a regression when local cover images are enabled. Specifically, if you bring up the shelf browser, then travel on it in either direction, local covers aren't displayed in the shelf browser. Unfortunately, just adding "KOHA.LocalCover.GetCoverFromBibnumber(true);" to the AJAX success handler isn't enough, as doing that also causes a duplicate of the main record's local cover image to be inserted each time you advance the shelf browser. I have not yet tested with any other cover image provider. I should emphasize that I really like the idea of this patch; it just needs to handle cover images better.
(In reply to Galen Charlton from comment #20) > This is close, but I found a regression when local cover images are enabled. > Specifically, if you bring up the shelf browser, then travel on it in either > direction, local covers aren't displayed in the shelf browser. > > Unfortunately, just adding "KOHA.LocalCover.GetCoverFromBibnumber(true);" to > the AJAX success handler isn't enough, as doing that also causes a duplicate > of the main record's local cover image to be inserted each time you advance > the shelf browser. > > I have not yet tested with any other cover image provider. > > I should emphasize that I really like the idea of this patch; it just needs > to handle cover images better. Galen, I cannot reproduce your issue. The pref OPACLocalCoverImages is enabled, I go on a detail page, click on "Browse shelf". I play with the left/right arrow and the local cover image still appears.
> > Specifically, if you bring up the shelf browser, then travel on it in either > > direction, local covers aren't displayed in the shelf browser. I can reproduce this problem in Firefox and Chrome on Windows. If I start from a record that has a local cover, the local cover image appears when I first open the shelf browser. If I click forward and then back again, the local cover no longer appears where it did.
Created attachment 21508 [details] [review] Bug 10856: Fix cover display in shelf browser This is an attempt to fix the bug discovered by Galen. I think I have enabled all "cover image" syspref and all seems to work well, but please test! :)
Thanks Julian!
Patch applied cleanly, go forth and signoff
Created attachment 21666 [details] [review] [SIGNED-OFF] Bug 10856: Fix cover display in shelf browser Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 21670 [details] [review] [PASSED QA] Bug 10856: Improve the previous and next items on the shelf browser The next and previous links should completely refresh the shelf. For example: [<] [1] [2] [3] [4] [5] [6] [>] Before this patch, the next and previous links were the same as the 1 and 6. With this patch, after clicking on next, we will get: [<] [7] [8] [9] [10] [11] [12] [13] [>] This patch adds a new ajax script to get the shelf browser block. Test plan: - On a detail biblio page, click on a "Browse shelf" link. - Play with the next and previous links. - Deactivate Javascript (using NoScript for example) and check that you get the same behavior (but the page is reloaded). - Launch the unit tests: prove t/db_dependent/ShelfBrowser.t Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script.
Created attachment 21671 [details] [review] [PASSED QA] Bug 10856: If callnumbers are equals, order should be on itemnumbers If a lot of items has the same callnumber, the order should be on the itemnumbers. Otherwise the left side is always filled with the same items. + Fix a bad c/p for the next link (when js is disabled). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 21672 [details] [review] [PASSED QA] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser In Firefox at least, the shelf browser cannot be reopened after hiding it with the "close shelf browser" link. This followup improves the behavior of the "close shelf browser" link so that the shelf browser can be redisplayed. To test, open a bibliographic detail page in the OPAC and click a "browse shelf" link. Click the "close shelf browser" link--the shelf browser should be hidden. Click the original "browse shelf" link and the shelf browser should reappear without reloading the page. Test with Firefox and Chrome (at least). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Testing notes: - New unit tests in t/db_dependent/ShelfBrowser.t pass. - System preference OPACShelfBrowser still works as expected. - Closing and opening the shelf browser works as expected. - Next and Previous links show new and nicer behaviour. - Logs are clean. Tested with Firefox and Chromium under Ubuntu. Notes: The currently displayed record could maybe be highlighted a bit better. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 21673 [details] [review] [PASSED QA] Bug 10856: Fix cover display in shelf browser Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> I was able to reproduce the problem with local covers and the patch fixes it in my tests.
Pushed to master. Thanks, Jonathan!