We have sticky toolbars throughout Koha but this is not yet supported in Vue apps. The Toolbar component should have a sticky mode enabled
Created attachment 176545 [details] [review] Bug 38899: Add a sticky toolbar This patch adds a sticky toolbar to the vendors form Test plan: N.B: This patchset will not work on a sandbox 1) Navigate to Acquisitions and click New vendor 2) The toolbar should appear at the top of the page underneath Add vendor 3) Scroll down, the toolbar should stick and be given the 'floating' class 4) Scroll back up and the toolbar should unstick and lose the 'floating' class
Created attachment 176546 [details] [review] Bug 38899: Add a cypress test
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/vue/components/Vendors/VendorFormAdd.vue). error: could not build fake ancestor Could be my problem?
Needs a rebase! I'll pick that up when I get a chance
We need that for 25.05 IMO, it's a regression (on the vendor edit page).
Created attachment 181822 [details] [review] Bug 38899: Add a sticky toolbar This patch adds a sticky toolbar to the vendors form Test plan: N.B: This patchset will not work on a sandbox 1) Navigate to Acquisitions and click New vendor 2) The toolbar should appear at the top of the page underneath Add vendor 3) Scroll down, the toolbar should stick and be given the 'floating' class 4) Scroll back up and the toolbar should unstick and lose the 'floating' class
Created attachment 181823 [details] [review] Bug 38899: Add a cypress test
What is required to make this work so I can test it? At the moment, after applying bug 38010 and doing a yarn build, restart_all, and using a browser with the cache cleared, I get a 404 page not found when going to Acquisitions > +New vendor. It might be obvious if you are a Vue developer what is required, but not to me 8-). Testing notes so far (using KTD): 1. This bug is related to bug 38010 - migrating vendors in the acquisitions module to using Vue. 2. Before bug 38010, the toolbar when adding a vendor (Acquisitions > + New vendor) was sticky. 3. After bug 38010, the toolbar is no longer sticky, as there is no sticky component/code in the Koha Vue code - this bug adds that. (My understanding so far.) 4. Before applying any patches, go to add a vendor in the acquisitions module (Acquisitions > + New vendor). 5. Note that as you scroll down the page, a sticky menu appears. 6. Apply the patches (and dependencies) for bug 38010. 7. Rebuild things: yarn build 8. Restart everything: restart_all
Hi David, there is an apache redirect that needs to be in place - a reset_all will do this for you
(In reply to Matt Blenkinsop from comment #9) > Hi David, there is an apache redirect that needs to be in place - a > reset_all will do this for you Thanks Matt!
Created attachment 181851 [details] [review] Bug 38899: Add a sticky toolbar This patch adds a sticky toolbar to the vendors form Test plan: N.B: This patchset will not work on a sandbox 1) Navigate to Acquisitions and click New vendor 2) The toolbar should appear at the top of the page underneath Add vendor 3) Scroll down, the toolbar should stick and be given the 'floating' class 4) Scroll back up and the toolbar should unstick and lose the 'floating' class Signed-off-by: David Nind <david@davidnind.com>
Created attachment 181852 [details] [review] Bug 38899: Add a cypress test Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. This bug is related to bug 38010 - migrating vendors in the acquisitions module to using Vue. 2. Before bug 38010, the toolbar when adding a vendor (Acquisitions > + New vendor) was sticky. 3. After bug 38010, the toolbar is no longer sticky, as there is no sticky component/code in the Koha Vue code - this bug adds that. 4. Before applying any patches, go to add a vendor in the acquisitions module (Acquisitions > + New vendor). 5. Note that as you scroll down the page, a sticky menu appears. 6. Apply the patches (and dependencies) for bug 38010. 7. Reset everything (required for an Apache redirect): reset_all 8. Go to add a vendor in the acquisitions module (Acquisitions > + New vendor). 7. Note that as you scroll down the page, the sticky menu in step 5 no longer appears. 8. Apply the patches for this bug (38899). 9. Rebuild things: yarn build 10. Restart everything: restart_all 11. Go to add a vendor in the acquisitions module (Acquisitions > + New vendor). 12. Note that the sticky toolbar is back!
13. The tests should pass: yarn cypress run --spec t/cypress/integration/Toolbar_spec.ts
Is this just me being dumb? jquery-migrate-3.3.2.min_24.1200036.js:2 JQMIGRATE: Migrate is installed, version 3.3.2 flatpickr.min_24.1200036.js:2 TypeError: Cannot read properties of undefined (reading 'appendChild') at Object.onReady (shortcut-buttons-flatpickr.min_24.1200036.js:2:3024) at pe (flatpickr.min_24.1200036.js:2:34535) at flatpickr.min_24.1200036.js:2:48317 at E (flatpickr.min_24.1200036.js:2:48331) at k (flatpickr.min_24.1200036.js:2:48618) at jQuery.fn.jQuery.fn.flatpickr (flatpickr.min_24.1200036.js:2:49426) at apply_flatpickr (acqui-home.pl:1585:7166) at HTMLInputElement.<anonymous> (acqui-home.pl:1585:7823) at S.each (jquery-3.6.0.min_24.1200036.js:2:3003) at s.fn.init.each (jquery-3.6.0.min_24.1200036.js:2:1481) k @ flatpickr.min_24.1200036.js:2 jQuery.fn.jQuery.fn.flatpickr @ flatpickr.min_24.1200036.js:2 apply_flatpickr @ acqui-home.pl:1585 (anonymous) @ acqui-home.pl:1585 each @ jquery-3.6.0.min_24.1200036.js:2 each @ jquery-3.6.0.min_24.1200036.js:2 (anonymous) @ acqui-home.pl:1585 e @ jquery-3.6.0.min_24.1200036.js:2 t @ jquery-3.6.0.min_24.1200036.js:2 setTimeout (anonymous) @ jquery-3.6.0.min_24.1200036.js:2 c @ jquery-3.6.0.min_24.1200036.js:2 fireWith @ jquery-3.6.0.min_24.1200036.js:2 fire @ jquery-3.6.0.min_24.1200036.js:2 c @ jquery-3.6.0.min_24.1200036.js:2 fireWith @ jquery-3.6.0.min_24.1200036.js:2 ready @ jquery-3.6.0.min_24.1200036.js:2 B @ jquery-3.6.0.min_24.1200036.js:2 flatpickr.min_24.1200036.js:2 TypeError: Cannot read properties of undefined (reading 'appendChild') at Object.onReady (shortcut-buttons-flatpickr.min_24.1200036.js:2:3024) at pe (flatpickr.min_24.1200036.js:2:34535) at flatpickr.min_24.1200036.js:2:48317 at E (flatpickr.min_24.1200036.js:2:48331) at k (flatpickr.min_24.1200036.js:2:48618) at jQuery.fn.jQuery.fn.flatpickr (flatpickr.min_24.1200036.js:2:49426) at apply_flatpickr (acqui-home.pl:1585:7166) at HTMLInputElement.<anonymous> (acqui-home.pl:1585:7823) at S.each (jquery-3.6.0.min_24.1200036.js:2:3003) at s.fn.init.each (jquery-3.6.0.min_24.1200036.js:2:1481) k @ flatpickr.min_24.1200036.js:2 jQuery.fn.jQuery.fn.flatpickr @ flatpickr.min_24.1200036.js:2 apply_flatpickr @ acqui-home.pl:1585 (anonymous) @ acqui-home.pl:1585 each @ jquery-3.6.0.min_24.1200036.js:2 each @ jquery-3.6.0.min_24.1200036.js:2 (anonymous) @ acqui-home.pl:1585 e @ jquery-3.6.0.min_24.1200036.js:2 t @ jquery-3.6.0.min_24.1200036.js:2 setTimeout (anonymous) @ jquery-3.6.0.min_24.1200036.js:2 c @ jquery-3.6.0.min_24.1200036.js:2 fireWith @ jquery-3.6.0.min_24.1200036.js:2 fire @ jquery-3.6.0.min_24.1200036.js:2 c @ jquery-3.6.0.min_24.1200036.js:2 fireWith @ jquery-3.6.0.min_24.1200036.js:2 ready @ jquery-3.6.0.min_24.1200036.js:2 B @ jquery-3.6.0.min_24.1200036.js:2 AcquisitionsMenu.vue:309 [Vue warn]: injection "permissionsStore" not found. at <App ordermanage=1 orderreceive=1 edifact=0 ... > warn @ runtime-core.esm-bundler.js:38 inject @ runtime-core.esm-bundler.js:1646 setup @ AcquisitionsMenu.vue:309 callWithErrorHandling @ runtime-core.esm-bundler.js:155 setupStatefulComponent @ runtime-core.esm-bundler.js:7204 setupComponent @ runtime-core.esm-bundler.js:7159 mountComponent @ runtime-core.esm-bundler.js:5508 processComponent @ runtime-core.esm-bundler.js:5483 patch @ runtime-core.esm-bundler.js:5085 render @ runtime-core.esm-bundler.js:6244 render @ runtime-dom.esm-bundler.js:1548 _update @ runtime-dom.esm-bundler.js:591 resolve @ runtime-dom.esm-bundler.js:544 _resolveDef @ runtime-dom.esm-bundler.js:551 connectedCallback @ runtime-dom.esm-bundler.js:478 (anonymous) @ islands.ts:110 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 Promise.then asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ islands.ts:103 (anonymous) @ islands.ts:103 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 requestIdleCallback hydrate @ islands.ts:87 (anonymous) @ acqui-home.pl:1187 islands.ts:110 [Vue warn]: Unhandled error during execution of setup function at <App ordermanage=1 orderreceive=1 edifact=0 ... > warn @ runtime-core.esm-bundler.js:38 logError @ runtime-core.esm-bundler.js:212 handleError @ runtime-core.esm-bundler.js:204 callWithErrorHandling @ runtime-core.esm-bundler.js:158 setupStatefulComponent @ runtime-core.esm-bundler.js:7204 setupComponent @ runtime-core.esm-bundler.js:7159 mountComponent @ runtime-core.esm-bundler.js:5508 processComponent @ runtime-core.esm-bundler.js:5483 patch @ runtime-core.esm-bundler.js:5085 render @ runtime-core.esm-bundler.js:6244 render @ runtime-dom.esm-bundler.js:1548 _update @ runtime-dom.esm-bundler.js:591 resolve @ runtime-dom.esm-bundler.js:544 _resolveDef @ runtime-dom.esm-bundler.js:551 connectedCallback @ runtime-dom.esm-bundler.js:478 (anonymous) @ islands.ts:110 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 Promise.then asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ islands.ts:103 (anonymous) @ islands.ts:103 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 requestIdleCallback hydrate @ islands.ts:87 (anonymous) @ acqui-home.pl:1187 runtime-core.esm-bundler.js:218 Uncaught TypeError: Cannot destructure property 'isUserPermitted' of 'permissionsStore' as it is undefined. at setup (AcquisitionsMenu.vue:310:17) at callWithErrorHandling (runtime-core.esm-bundler.js:155:1) at setupStatefulComponent (runtime-core.esm-bundler.js:7204:1) at setupComponent (runtime-core.esm-bundler.js:7159:1) at mountComponent (runtime-core.esm-bundler.js:5508:1) at processComponent (runtime-core.esm-bundler.js:5483:1) at patch (runtime-core.esm-bundler.js:5085:1) at Object.render (runtime-core.esm-bundler.js:6244:1) at render (runtime-dom.esm-bundler.js:1548:1) at HTMLElement._update (runtime-dom.esm-bundler.js:591:1) setup @ AcquisitionsMenu.vue:310 callWithErrorHandling @ runtime-core.esm-bundler.js:155 setupStatefulComponent @ runtime-core.esm-bundler.js:7204 setupComponent @ runtime-core.esm-bundler.js:7159 mountComponent @ runtime-core.esm-bundler.js:5508 processComponent @ runtime-core.esm-bundler.js:5483 patch @ runtime-core.esm-bundler.js:5085 render @ runtime-core.esm-bundler.js:6244 render @ runtime-dom.esm-bundler.js:1548 _update @ runtime-dom.esm-bundler.js:591 resolve @ runtime-dom.esm-bundler.js:544 _resolveDef @ runtime-dom.esm-bundler.js:551 connectedCallback @ runtime-dom.esm-bundler.js:478 (anonymous) @ islands.ts:110 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 Promise.then asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ islands.ts:103 (anonymous) @ islands.ts:103 step @ index.mjs:34 (anonymous) @ index.mjs:34 asyncGeneratorStep @ index.mjs:34 _next @ index.mjs:34 (anonymous) @ index.mjs:34 (anonymous) @ index.mjs:34 requestIdleCallback hydrate @ islands.ts:87 (anonymous) @ acqui-home.pl:1187 And also: Sticky toolbar 1) Should stick on scroll 0 passing (3s) 1 failing 1) Sticky toolbar Should stick on scroll: TypeError: The following error originated from your application code, not from Cypress. > Cannot destructure property 'isUserPermitted' of 'permissionsStore' as it is undefined. When Cypress detects uncaught errors originating from your application it will automatically fail the current test. This behavior is configurable, and you can choose to turn this off by listening to the `uncaught:exception` event. https://on.cypress.io/uncaught-exception-from-application at setup (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/acquisitions-menu.09f785f53cd6ce2fae1b.esm.js:99:17) at callWithErrorHandling (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:3673:22) at setupStatefulComponent (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:10706:29) at setupComponent (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:10661:11) at mountComponent (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:9012:13) at processComponent (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:8987:17) at patch (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:8589:21) at Object.render (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:9748:13) at render (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:13085:22) at HTMLElement._update (http://localhost:8081/intranet-tmpl/prog/js/vue/dist/islands.esm_24.1200036.js:12128:9) I am unsure whether I applied something wrong, or whether this is a real failure? Setting this to FQA, please reset to 'Signed off' if I made a mistake somewhere: - Applied this bug and all dependencies onto current upstream/main - reset_all as suggested - yarn build - restart_all - Manually tested stickyness (which works and is not the reason for FQA) - Saw the above mentioned errors + warnings in the console acqui-home.pl - Ran the cypress spec, ran into above failure
It works for me, however can we make it match the patron toolbar? https://snipboard.io/VBxNCj.jpg vs https://snipboard.io/v2JSHZ.jpg 1. Missing "save" icon 2. Cancel misaligned and not green (appears grey)
Created attachment 181965 [details] [review] Bug 38899: (QA follow-up) Match buttons to patron toolbar
Looks good, works good. Do we need to tidy vue and ts files? FAIL koha-tmpl/intranet-tmpl/prog/js/vue/components/ButtonSubmit.vue FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/js/vue/components/ButtonSubmit.vue` FAIL koha-tmpl/intranet-tmpl/prog/js/vue/modules/acquisitions.ts FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/js/vue/modules/acquisitions.ts`
Created attachment 182151 [details] [review] Bug 38899: (QA follow-up) Match buttons to patron toolbar
Tidied and squashed to last commit - not sure why the pre-commit hook tidy didn't catch those but QA tests are passing now
Still noticing a tiny difference between the 2 versions of the toolbar (comparing vendor edit and patron edit): I think it's coming from the use of <font-awesome-icon> vs <i>. Actually the Vue version is better (the vertical alignment) now IMO :)
There is something wrong. Cypress tests are now failing, because you renamed #vendor_add with #add_vendor. But renaming in the test is not enough, as I think it's not consistent: * the naming is not "add_vendor" vs "vendors_list", etc. * the block on which the id is is not consistent: main #vendors_list #toolbar .page-section the content vs main h1 #add_vendor #toolbar the content
Created attachment 182249 [details] [review] Bug 38899: Fix cypress tests
Created attachment 182250 [details] [review] Bug 38899: Move the toolbar out of the form
Well, the tests are still failing. AssertionError: Timed out retrying after 10000ms: Expected to find content: 'Submit' within the element: <form#add_vendor> but never did.
Created attachment 182255 [details] [review] Bug 38899: Improve HTML consistency
(In reply to Matt Blenkinsop from comment #26) > Created attachment 182255 [details] [review] [review] > Bug 38899: Improve HTML consistency No, this is wrong. We now have 2 "main". I think we should have: main #vendors_add #toolbar .page-section form
Created attachment 182449 [details] [review] Bug 38899: Remove code duplication
Where are we at here? I think this is still needed for 25.05?
Created attachment 182580 [details] [review] Bug 38899: Restore id attribute to the form
This is the closest I can get with the stickyness still working - if I replace the template tag with a div, the toolbar just scrolls up with the form
I'm still seeing failing cypress tests for this I'm afraid. Vendors in particular.
Created attachment 182646 [details] [review] Bug 38899: Fix cypress tests
Let's aim for this one early next cycle?
I've changed this from an enhancement to a normal bug. While it is an enhancement to Vue, it restores something that existed before, for vendors in the acquisitions module.
Created attachment 182973 [details] [review] Bug 38899: Add a sticky toolbar This patch adds a sticky toolbar to the vendors form Test plan: N.B: This patchset will not work on a sandbox 1) Navigate to Acquisitions and click New vendor 2) The toolbar should appear at the top of the page underneath Add vendor 3) Scroll down, the toolbar should stick and be given the 'floating' class 4) Scroll back up and the toolbar should unstick and lose the 'floating' class Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182974 [details] [review] Bug 38899: Add a cypress test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182975 [details] [review] Bug 38899: (QA follow-up) Match buttons to patron toolbar
Created attachment 182976 [details] [review] Bug 38899: Move the toolbar out of the form
Created attachment 182977 [details] [review] Bug 38899: Improve HTML consistency
Created attachment 182978 [details] [review] Bug 38899: Remove code duplication
Created attachment 182979 [details] [review] Bug 38899: Restore id attribute to the form
Created attachment 182980 [details] [review] Bug 38899: Fix cypress tests
Created attachment 183114 [details] [review] Bug 38899: Add a sticky toolbar This patch adds a sticky toolbar to the vendors form Test plan: N.B: This patchset will not work on a sandbox 1) Navigate to Acquisitions and click New vendor 2) The toolbar should appear at the top of the page underneath Add vendor 3) Scroll down, the toolbar should stick and be given the 'floating' class 4) Scroll back up and the toolbar should unstick and lose the 'floating' class Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183115 [details] [review] Bug 38899: Add a cypress test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183116 [details] [review] Bug 38899: (QA follow-up) Match buttons to patron toolbar Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183117 [details] [review] Bug 38899: Move the toolbar out of the form Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183118 [details] [review] Bug 38899: Improve HTML consistency Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183119 [details] [review] Bug 38899: Remove code duplication Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183120 [details] [review] Bug 38899: Restore id attribute to the form Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 183121 [details] [review] Bug 38899: Fix cypress tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Nice work everyone! Pushed to main for 25.11