Extract a DestinationSelectorComponent #1727
Conversation
jcoyne
force-pushed
the
destination-component
branch
5 times, most recently
from
ebca193 to
9162919
Compare
jcoyne
force-pushed
the
destination-component
branch
4 times, most recently
from
6850493 to
3b32b61
Compare
| <%= label_for_pickup_destinations_dropdown(current_request.pickup_destinations) %> | ||
| Will be delivered to |
There was a problem hiding this comment.
Is this (and the test changes) an intentional change from the current behavior? I think the subtle intent is to indicate whether the patron has any choice about the delivery location, and it seems like we've lost that?
This wording has a long and elaborate history. It's possible with SPEC/Rumsey/etc using Aeon, we can revisit the wording.. but I'm a little uncomfortable if we do it in a refactoring PR.
There was a problem hiding this comment.
This seemed weird that the status display would display two different labels "Will be delivered to" and "Delivered to" depending on whether or not there was a choice. The function that was used here was intended as a label for a dropdown, but this is not a dropdown.
Do you want to retain this behavior? I can revert this part, but it just seems wrong.
There was a problem hiding this comment.
I would err on the side of keeping the current behavior. It was important at the time and I think there's a UX argument to be made about using consistent labels between the form and the email.
If we want to change it, I think it's important to do so transparently and not in a refactoring PR.
jcoyne
force-pushed
the
destination-component
branch
from
3b32b61 to
180d2d0
Compare
|
@cbeer reverted the change on the summary page. |
jcoyne
force-pushed
the
destination-component
branch
from
180d2d0 to
d8dd7dc
Compare
From https://github.com/sul-dlss/sul-requests/pull/1638\#discussion_r1293674040