Created attachment 143176 [details] [review]
Bug 32108: Replace "x" icon with "trash" + Remove

Change "x" icon to replace patron when scheduling a pickup.

Test plan:
Schedule a new pickup, select a patron, notice the change.

Created attachment 143180 [details] [review]
Bug 32108: Replace "x" icon with "trash" + Remove

Change "x" icon to replace patron when scheduling a pickup.

Test plan:
Schedule a new pickup, select a patron, notice the change.

Signed-off-by: David Nind <david@davidnind.com>

Testing notes (using koha-testing-docker).

Since I haven't used the curbside pick up before, these are my notes on setting things up for testing:
1. Enable the CurbsidePickup system preference.
2. Setup dates and times when pickups are available;
   2.1 Go to Administration > Patrons and circulation > Curbside pickup
   2.2 For a library (for example Centerville) click on enable, pickup interval = 5, maximum patrons per interval = 2, enable for waiting holds only, and add some slots, and save.
3. Place some holds for a patron for pickup at the library enabled.
4. Check items in and fulfil the holds.
5. Schedule a pcikup time for the patron:
   5.1 Go to Circulation > Holds > Curbside pickups.
   5.2 Select the schedule pickup tab.
   5.3 Start typing the patron's name and select.
6. Next to the patron name is a red X before the patch.
7. Apply the patch and refresh the page.
8. The red X is replaced with a green trash can and Remove.

I like the x icon better than the trash icon, because it does empty the field but not actually delete/remove. I think the red color was restored by another patch I tested, but not sure which one right now :(

(In reply to Katrin Fischer from comment #4)
> I like the x icon better than the trash icon, because it does empty the
> field but not actually delete/remove. I think the red color was restored by
> another patch I tested, but not sure which one right now :(

As said in the commit message I have simply copied the behaviour from other areas (like the guarantor when editing a patron).
Shouldn't we reach consistency first, then improve if we want it?

Should we actually have a "search" icon? The tooltip of the icon says "Search for another patron".

Created attachment 143353 [details] [review]
[ALTERNATIVE PATCH] Bug 32108: Replace "x" icon with the "search" icon

Change "x" icon to replace patron when scheduling a pickup.

Test plan:
Schedule a new pickup, select a patron, notice the change.

How it looks like with the alternative patch: https://snipboard.io/YsFVnT.jpg

(In reply to Jonathan Druart from comment #8)
> How it looks like with the alternative patch: https://snipboard.io/YsFVnT.jpg

Sorry, I think I was too much in a hurry here - I thought you wanted to replace the X in general. But: I really like the change to the search icon.

Is it expected that the search doesn't work with cardnumber? Or is it because 42 is too short an input?

Created attachment 143611 [details] [review]
Bug 32108: Replace "x" icon with the "search" icon

Change "x" icon to replace patron when scheduling a pickup.

Test plan:
Schedule a new pickup, select a patron, notice the change.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

(In reply to Katrin Fischer from comment #10)
> Is it expected that the search doesn't work with cardnumber? Or is it
> because 42 is too short an input?

The autocomplete starts at 3 chars, yes. Is that a problem?

Is this PQA or In discussion?

(In reply to Jonathan Druart from comment #12)
> (In reply to Katrin Fischer from comment #10)
> > Is it expected that the search doesn't work with cardnumber? Or is it
> > because 42 is too short an input?
> 
> The autocomplete starts at 3 chars, yes. Is that a problem?

I think it's at least a totally different issue than the button :)

(In reply to Tomás Cohen Arazi from comment #13)
> Is this PQA or In discussion?

I feel the search icon is fine, so no disagreement on my side, we could ask for another opinion if you think it would be better.

Pushed to master for 22.11.

Nice work everyone, thanks!