Bug 33491 - EBSCO Packages - Add new agreement UI has some issues
Summary: EBSCO Packages - Add new agreement UI has some issues
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
: 35132 (view as bug list)
Depends on: 33483
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-11 16:57 UTC by Pedro Amorim
Modified: 2023-10-23 15:05 UTC (History)
8 users (show)

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


Attachments
screenshot of UI (72.86 KB, image/jpeg)
2023-04-11 16:57 UTC, Pedro Amorim
Details
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements (1.12 KB, patch)
2023-04-11 19:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements (1.23 KB, patch)
2023-04-17 20:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements (1.31 KB, patch)
2023-04-18 16:31 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements (1.36 KB, patch)
2023-04-19 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
comparison (33.86 KB, image/png)
2023-04-20 09:25 UTC, Pedro Amorim
Details
Bug 33491: Do not apply style to agreement list view (1.40 KB, patch)
2023-05-02 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33491: Do not apply style to agreement list view (1.45 KB, patch)
2023-05-02 14:05 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33491: Add a more specific class for the modal (1.66 KB, patch)
2023-05-03 07:06 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 Pedro Amorim 2023-04-11 16:57:20 UTC
Created attachment 149453 [details]
screenshot of UI
Comment 1 Pedro Amorim 2023-04-11 16:58:52 UTC
* There's a problem with the "filter by mine" filter at the top
* There's a problem with the table not going full width, this is not as trivial as the fix I did for bug 33066 here https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=33066&attachment=149183

Can't just move the table out of the <fieldset> since it's a nested component.
Comment 2 Jonathan Druart 2023-04-11 19:39:20 UTC
Created attachment 149474 [details] [review]
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements
Comment 3 Jonathan Druart 2023-04-11 19:40:18 UTC
This patch is not ideal but it's fixing the main problems. Pedro, do you think it's acceptable as it, at least for backport maybe then we see how we can improve this view later?
Comment 4 Pedro Amorim 2023-04-12 16:59:27 UTC
It doesn't fix the "show mine only" being punted to the left, this is because it's getting a float:left also from being a child of <fieldset> (which is also causing the table to not be full width).

The problem is <EHoldingsPackageAgreements> sits inside a <fieldset> in EHoldingsEBSCOPackagesShow.vue.

Then, in EHoldingsEBSCOPackageAgreements.vue we're opening the modal that has <AgreementsList> inside it, but that modal is already a child of the aforementioned <fieldset> so it's inheriting its style unintendendly.

There is also another problem: the modal needs a overflow (and perhaps a max-height?). If we have many agreements created, the modal overflows the screen and doesn't let the user scroll vertically.
Comment 5 Jonathan Druart 2023-04-17 20:24:47 UTC
Created attachment 149778 [details] [review]
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements
Comment 6 Jonathan Druart 2023-04-17 20:25:29 UTC
(In reply to Pedro Amorim from comment #4)
> It doesn't fix the "show mine only" being punted to the left, this is
> because it's getting a float:left also from being a child of <fieldset>
> (which is also causing the table to not be full width).
> 
> The problem is <EHoldingsPackageAgreements> sits inside a <fieldset> in
> EHoldingsEBSCOPackagesShow.vue.
> 
> Then, in EHoldingsEBSCOPackageAgreements.vue we're opening the modal that
> has <AgreementsList> inside it, but that modal is already a child of the
> aforementioned <fieldset> so it's inheriting its style unintendendly.

Fixed in the last version of the patch.

> There is also another problem: the modal needs a overflow (and perhaps a
> max-height?). If we have many agreements created, the modal overflows the
> screen and doesn't let the user scroll vertically.

I would suggest to deal with that on a separate bug. I think we need a separate component for modal, in which we will deal with this issue (we use .modal in a couple of places already)
Comment 7 Matt Blenkinsop 2023-04-18 16:31:29 UTC
Created attachment 149823 [details] [review]
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-04-19 09:40:41 UTC
Created attachment 149853 [details] [review]
Bug 33491: Fix UI issues in EHoldingsEBSCOPackageAgreements

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2023-04-19 09:41:08 UTC
This is certainly an improvement, Passing QA.
Comment 10 Tomás Cohen Arazi 2023-04-19 12:22:22 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 11 Pedro Amorim 2023-04-20 09:25:40 UTC
Created attachment 149924 [details]
comparison

Top picture is the header in agreements list before the patch, bottom picture is after the patch.

This CSS in this patch is being applied in agreements list unintentionally.

.filters input[type="checkbox"] {
    margin-left: 0 !important;
}
Comment 12 Jonathan Druart 2023-05-02 08:21:03 UTC
Created attachment 150505 [details] [review]
Bug 33491: Do not apply style to agreement list view

We removed the 'scoped' attribute and so the style rules added in the
previous patch were applied to the agreement list view.
Why is 'scoped' not working is the main question here (?) but adding a
more specific selector to aim only the component (AgreementsList) from
the modal is a quick solution.
Comment 13 Jonathan Druart 2023-05-02 08:21:29 UTC
(In reply to Pedro Amorim from comment #11)
> Created attachment 149924 [details]
> comparison
> 
> Top picture is the header in agreements list before the patch, bottom
> picture is after the patch.
> 
> This CSS in this patch is being applied in agreements list unintentionally.
> 
> .filters input[type="checkbox"] {
>     margin-left: 0 !important;
> }

This patch is fixing the problem. Do you have something better to suggest?
Comment 14 Pedro Amorim 2023-05-02 14:05:22 UTC
Created attachment 150518 [details] [review]
Bug 33491: Do not apply style to agreement list view

We removed the 'scoped' attribute and so the style rules added in the
previous patch were applied to the agreement list view.
Why is 'scoped' not working is the main question here (?) but adding a
more specific selector to aim only the component (AgreementsList) from
the modal is a quick solution.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 15 Jonathan Druart 2023-05-02 14:25:07 UTC
> Why is 'scoped' not working is the main question here

As Pedro pointed to me, it is actually working as expected: style rules are defined in EHoldingsEBSCOPackageAgreements and the table is in AgreementList.
Comment 16 Jonathan Druart 2023-05-03 07:05:15 UTC
There is another problem: https://snipboard.io/Odhrai.jpg
The .modal rule should apply to the modal of the EHoldingsEBSCOPackageAgreements component only.
Comment 17 Jonathan Druart 2023-05-03 07:06:10 UTC
Created attachment 150564 [details] [review]
Bug 33491: Add a more specific class for the modal

We don't want to apply these CSS rules to other modals
Comment 18 Jacob O'Mara 2023-05-09 20:52:46 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 19 Jacob O'Mara 2023-05-10 15:03:46 UTC
PLEASE IGNORE THE ABOVE, NOT PUSHED TO STABLE
Comment 20 Jonathan Druart 2023-05-10 15:05:31 UTC
(In reply to Jonathan Druart from comment #17)
> Created attachment 150564 [details] [review] [review]
> Bug 33491: Add a more specific class for the modal
> 
> We don't want to apply these CSS rules to other modals

Pedro, should we go with this patch and see if we can do better on a separate bug?
Comment 21 Martin Renvoize 2023-05-11 16:19:29 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 22 Lucas Gass 2023-05-15 14:43:21 UTC
Missing dependencies for 22.05.x, no backport
Comment 23 Jonathan Druart 2023-06-09 15:25:35 UTC
Last patch still missing.
Comment 24 Tomás Cohen Arazi 2023-06-09 16:34:02 UTC
(In reply to Jonathan Druart from comment #23)
> Last patch still missing.

Sorry, I merged the last two patches. Don't remember how/why.
Comment 25 Matt Blenkinsop 2023-10-23 15:05:38 UTC
*** Bug 35132 has been marked as a duplicate of this bug. ***