Modals in the Vue components (ERM, Preservation at least) are not displayed nicely. From bug 37733 comment 0 """ 2. "Add to waiting list" modal is not centered http://localhost:8081/cgi-bin/koha/preservation/waiting-list https://snipboard.io/mIBYkj.jpg 3. Confirmation box "Are you sure you want to remove this item" is not centered correctly https://snipboard.io/ZgJFGC.jpg """
Created attachment 170956 [details] [review] Bug 37812: Try to fix modal in the Preservation module We certainly need more as ERM module also use modals. And the Dialog component seems to have more elements affected.
I also started converting some from dialog to actual modal in Bug 37605 - Why do the vue apps use 'dialog' in place of modals?
Created attachment 171252 [details] [review] Bug 37812: Adjust modals from the Preservation module - bootstrap5
Created attachment 171253 [details] [review] Bug 37812: Adjust Dialog.vue component
This is how we should fix it IMO. However there are still quite a lot of work to do. https://snipboard.io/TJ9zek.jpg => OK https://snipboard.io/MQHoCt.jpg => Dropdown list's options are hidden behind the modal's footer https://snipboard.io/5ru41B.jpg => Could be better styled Do we want to go with toasts? (it's not introduced in Koha yet so I don't know if it's a good idea). There are also some "close"/"x" buttons that does not close/hide the modal. However the code is similar with other places where it is working... ? For discussion, please help!
And I forgot this one: https://snipboard.io/rY90KF.jpg
This must be part of 24.11. At least the preservation module is unusable.
(In reply to Jonathan Druart from comment #5) > Do we want to go with toasts? (it's not introduced in Koha yet so I don't > know if it's a good idea). Moved the discussion to its own bug, see bug 37939.
How can I trigger this modal? > https://snipboard.io/MQHoCt.jpg => Dropdown list's options are hidden behind the modal's footer
Created attachment 171604 [details] [review] Bug 37812: Prevent "underflow" when using comboboxes in bootstrap modals
Created attachment 171605 [details] [review] Bug 37812: Improve consistency of modal styling in Dialog.vue - Use modal-header for content, modal-footer for acknowledgement in alert type modal. - Conditionally render modal-body if message or inputs available. - Use modal footer for interactions and hide top border if body wasn't rendered.
Created attachment 171625 [details] [review] Bug 37812: Prevent non-functional close buttons by using vue event handlers
(In reply to Paul Derscheid from comment #12) > Created attachment 171625 [details] [review] [review] > Bug 37812: Prevent non-functional close buttons by using vue event handlers Can you explain why some are working and others not?
My suspicion that this has never worked is growing. I just rebuilt webpack using the old webpack config and without the last patch applied it doesn't work as expected either. I think what's happening is the following. Where the modals are in use they always hinge on a variable being defined (checked via v-if), if that variable is altered after an API call, the modal closes automatically. I need to look at more cases, so take this w/ a grain a salt.
Created attachment 171641 [details] [review] Bug 37812: Prevent initialisation failure by using modal methods, not plain show, hide
You were right joubu, it's not what I said in comment #14. It's so easy to miss and I kept looking in the wrong place. The simple reason that this failed in TrainsShow.vue was that the modal needs to be properly initialised by either data-bs-toggle="modal" or $(<id>).modal('show'), not plain $(<id>).show(). I finally saw the issue after I noticed that the backdrop was missing which screamed init failure.
Great, thanks a lot, Paul. Everything is working nicely now! The only thing I noticed it that the "warning" Dialogs do not close when clicking outside of the dialog. Not blocker ofc.
Strange, I'll take a look at that in a bit.
Created attachment 171651 [details] [review] Bug 37812: Fix a modal in the ERM module
I think we are good for now.
Created attachment 171690 [details] [review] Bug 37812: Adjust modals from the Preservation module - bootstrap5 Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171691 [details] [review] Bug 37812: Adjust Dialog.vue component Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171692 [details] [review] Bug 37812: Prevent "underflow" when using comboboxes in bootstrap modals Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171693 [details] [review] Bug 37812: Improve consistency of modal styling in Dialog.vue - Use modal-header for content, modal-footer for acknowledgement in alert type modal. - Conditionally render modal-body if message or inputs available. - Use modal footer for interactions and hide top border if body wasn't rendered. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171694 [details] [review] Bug 37812: Prevent non-functional close buttons by using vue event handlers Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171695 [details] [review] Bug 37812: Prevent initialisation failure by using modal methods, not plain show, hide Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171696 [details] [review] Bug 37812: Fix a modal in the ERM module Signed-off-by: David Nind <david@davidnind.com>
Created attachment 171697 [details] Screenshots of dialog boxes for the preservation and ERM modules - before and after Testing notes using KTD: 1. Enable the ERM and preservation modules . ERMModule = Enable . PreservationModule = Enable 2. Set up the preservation module for testing: 2.1 Go to Preservation > Settings. 2.2 For general settings: - Status for item added to waiting list: Not for loan 2.3 For Processings: - Add three processing steps called Step 1, Step 2, and Step 3: . Add new processing . Processing name: name of the step (Step 1, Step 2, and Step 3) . Submit 2.4 Add some items to the waiting list: - Waiting list > Add to waiting list - Use these barcodes: 39999000011418, 39999000000559, 39999000006155 2.5 Create a new train: - Trains > New train - Name: Train 1 - Description: Description - Default processing: Step 1 2.6 Add an item to a train - from the waiting list: - Waiting list > Add to waiting list - Immediatly after adding to the waiting list, select Add last 1 items to a train (unfer the "1 new items added." message) - Select a train and click Save 3. Set up the ERM module for testing: 3.1 Add a new agreement (Agreements > New agreement > add some information for the required fields) 3.2 Add a new license (Licenses > New license > add some information for the required fields) 3.3 Add a new package (eHoldings > Local > Packages > New package > add some information for the required fields) 3.4 Add a new title (eHoldings > Local > Titles > New title > add some information for the required fields) 3.5 Add a new data provider (eUsage > Data providers > New data provider > add some information for the required fields) 3.6 Create a report (eUsage > Reports > select the create report tab > add some information for the required fields > enter a report name and save the report) 4. Check these examples and note the display issues for the preservation module (see the before and after screenshots attachment to see the differences): 4.1 Delete a train (Trains > Delete (from the actions column)) 4.2 Add a new item to a train – not in the waiting list (Trains > Add items (from the actions column) > enter an invalid barcode such as 12345) 4.3 Remove an item from a train (Trains > select a train from the list of trains > Items > press the delete icon for an item) 4.4 Add items to the waiting list (Waiting list > Add to waiting list) 4.5 Remove an item from the waiting list (Waiting list > Remove (from the actions column)) 4.6 Remove a processing step (Settings > Processings > select Remove this processing for a step) 5. Check these examples and note the display issues for the ERM module (see the before and after screenshots attachment to see the differences): 5.1 Delete an agreement 5.2 Delete a license 5.3 Delete a package 5.4 Delete a title 5.5 Delete a data provider 5.6 Run a saved report 6. Apply the patches. 7. Rebuild the modules: yarn build 8. Restart everything: restart_all 9. Check the items in steps 4 and5, these should now display as they should and work as expected (see the before and after screenshots attachment).
Created attachment 171702 [details] [review] Bug 37812: Adjust modals from the Preservation module - bootstrap5 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171703 [details] [review] Bug 37812: Adjust Dialog.vue component Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171704 [details] [review] Bug 37812: Prevent "underflow" when using comboboxes in bootstrap modals Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171705 [details] [review] Bug 37812: Improve consistency of modal styling in Dialog.vue - Use modal-header for content, modal-footer for acknowledgement in alert type modal. - Conditionally render modal-body if message or inputs available. - Use modal footer for interactions and hide top border if body wasn't rendered. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171706 [details] [review] Bug 37812: Prevent non-functional close buttons by using vue event handlers Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171707 [details] [review] Bug 37812: Prevent initialisation failure by using modal methods, not plain show, hide Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171708 [details] [review] Bug 37812: Fix a modal in the ERM module Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Great teamwork here, thanks to all involved. Passing QA
(In reply to Paul Derscheid from comment #18) > Strange, I'll take a look at that in a bit. Maybe we should wait for you to have a look before pushing this one.
Just FYI, I know what the problem is with comment #18. Here it's the same thing as before. The modal is not properly initialised via the modal methods. I'm currently working on this, but I get a lot of cypress test failures, that I'm trying to fix first.
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
Ah, that shouldn't have been pushed yet, will cause some cypress tests to fail because they were not adjusted after one of my patches. That's what I meant by comment #38.
Created attachment 171758 [details] [review] Bug 37812: (QA follow-up) Prevent lots of cypress test failures by reinserting 'confirmation' class on modal header
That fixes 7/11. 4 to go.
Created attachment 171780 [details] [review] Bug 37812: (QA follow-up) Prevent cypress test failure by adjusting selector in DataProviders_spec.ts
Created attachment 171781 [details] [review] Bug 37812: (QA follow-up) Prevent cypress test failures by correcting modal selector, removing ambiguity of contains calls, replace by get with respective ids
That's all cypress tests fixed. But there's still one thing to do, to initialise the modal in Dialog.vue with the bootstrap methods.
Created attachment 171786 [details] [review] Bug 37812: (QA follow-up) Add watchers for warning and confirmation prop and initialise modals with bootstrap methods on changes in Dialog.vue - Use more targeted selectors in cypress tests Trains.ts, WaitingList.ts. - Await result of accept_callback, then close modal.
That was the last one! Ready to push (after a sanity check, of course). All cypress tests still pass locally for me.
Follow-ups pushed, thanks Paul.
Owen, Martin, I am wondering if the change "Submit" => "Save" for modals is correct. For instance we have "Add items to waiting list" and a "Save" button. Wasn't "Submit" more correct?
(In reply to Jonathan Druart from comment #49) > Owen, Martin, I am wondering if the change "Submit" => "Save" for modals is > correct. For instance we have "Add items to waiting list" and a "Save" > button. Wasn't "Submit" more correct? So... the edit train form has "Submit" (I would have expected "Save"). Went to the "normal Koha" and found that adding a library has "Submit" and editing a bib record has "Save". I guess at this point we just don't care, right? :D