Gift cards/add recipient #2412
Gift cards/add recipient #2412
Conversation
|
We have received a translation request but there is a question blocking translation work. Your help is needed. Please answer the following questions.
|
antiBaconMachine
force-pushed
the
gift-cards/add-recipient
branch
from
550be40
to
5494658
Compare
Implement the recipient form for gift card product
1. As a hidden form field with a value of "if_present". This will be used by clients without javascript. The 'if_present' flag gives a hint to the backend to only validate email addresses if one is given, blank values are acceptable 2. As a checkbox. This is a progressive enhancement which replaces the hidden field in 1 when javascript is enabled. The value sent to the backend will be on when the checkbox is checked. No field at all will be sent when it is unchecked. The backend will treat blank email as an error if the value is 'on' The control field is used by the backend to enable the send to recipient feature validation and processing. It must be present for the feature to do anything. We only need it to be present if the user has elected to fill out the form.
This was a good idea but unfortunately it doesn't work with liquid rendering flow, we need the passed in elements to render within the inner liquid context
The dummy form is generated because we want to use properties from the liquid product from drop. It doesn't serve any actual purpose in the html. Normally product forms are wrapped in a <product-form> custom element which have some styling applied. We don't want to do that because the custom element also adds a bunch of js that we don't want. Instead we just use a div with the class.
When js is disabled the form displays with no checkbox When js is enabled the form is hidden at page load and the checkbox is unhidden and enabled This works well but is susceptible to content flashes as the form is first shown then quickly hidden. Probably unacceptable. An obvious solution would be to check the checkbox by default which would mean there is no need to hide the form on load. A more extreme solution is to use a noscript tag to completely sever the link between the two sets of markup. I don't like this solution as it bloats the source but it does neatly take care of the content flash
… on wether the client has JS enabled
When we read errors out of the flash state we want them displayed nicely inline on the recipient form. We can do this by ripping off the pattern frolm customer registration. There are a few styles which have been implemented as specific .customer styles which should really be a generic class but not a huge issue to just use the customer class in our form as well
This is important as the field names we get from the backend are lower case and we use them to dynamically build selectors
We tried to simplify things by having the recipient form be part of the main product form but it didn't work. We can just use the html form attribute to reassign inputs from a different form to the main product form. Messy from a source view but nicer from a visual editor POV.
…ript Liquid errors already render nicely next to the fields they belong to. We'd eventually like the same behaviour in JS. That will require tweaking the backend error payload a bit but is otherwise achievable. We first have to disable the existing product form error handling as this appears next to the buy buttons Instead we publish the error response on the pub/sub bus and display the error at the top of the recipient form. We have liquid render the error container so it looks roughly the same regardless of whether the error is rendered by liquid or JS. Next step will be to break the error message down into fields on the backend and unpack the messages on the FE. Co-authored-by: Frederic Ma <frederic.ma@shopify.com>
There is no validation on these fields yet but there will be
Since this was introduced the markup has changed to introduce an extra form wrapper which takes care of what this was trying to achieve
Was throwing the alignment out
Co-authored-by: Ludo <ludo.segura@shopify.com>
This mirrors what liquid does to render errors.
antiBaconMachine
force-pushed
the
gift-cards/add-recipient
branch
from
ecb84c4
to
c9b9283
Compare
| {% render 'gift-card-recipient-form', product: product, form: form, section: section %} | ||
| {% endcomment %} | ||
|
|
||
| <div class="customer"> |
There was a problem hiding this comment.
What's this customer class doing?
There was a problem hiding this comment.
Oh I think that's something that needed tweaking. Because some styling applied on the customer pages matched what was applied here, I think this class was used but should be duplicated with a more accurate naming if it's still useful or remove if it's not necessary
There was a problem hiding this comment.
Yeah a lot of the form styling is lifted from the customer registration form, so we're using their classes. It's just about accurate as a gift card recipient does result in a customer being created.
| .recipient-form > input[type='checkbox'] { | ||
| position: absolute; | ||
| width: 1.6rem; | ||
| height: 1.6rem; | ||
| margin: var(--recipient-checkbox-margin-top) 0; | ||
| top: 0; | ||
| left: 0; | ||
| z-index: -1; | ||
| appearance: none; | ||
| -webkit-appearance: none; | ||
| } |
There was a problem hiding this comment.
It would be nice if any generic checkbox styles were added in a more global way. Though I recognize the facets checkbox styles were also unfortunately scoped specifically to that purpose, so I wouldn't say it's a hard requirement to change that here. Would probably increase testing scope if we moved any facet styles around too.
| .recipient-form > input[type='checkbox']:checked ~ .recipient-fields { | ||
| display: block; | ||
| animation: animateMenuOpen var(--duration-default) ease; | ||
| } |
There was a problem hiding this comment.
I wonder if we should provide more feedback to screenreader users that additional content gets revealed when checking the input 🤔 Maybe if we at least prevented the item from being successfully added to cart without a valid email this would be ok. But currently, it's possible you could check the checkbox and press enter to submit the form and add the item to the cart without noticing the additional fields. https://screenshot.click/17-02-h5lay-yhktj.mp4
There was a problem hiding this comment.
Same feeling on my side. I brought the possibility to add something like toggling an attribute aria-expanded="true".
video
There was a problem hiding this comment.
Yeah unfortunately I'm pretty sure aria-expanded isn't applicable to inputs like this. At least it's not meant to be. So I was picturing it having to be more of a live region type solution.
There was a problem hiding this comment.
Maybe something on which we could get @metamoni's take 👍
There was a problem hiding this comment.
Yeah I did some quick experiments with aria-expanded and concluded that it was the wrong tool for the job.
antiBaconMachine
force-pushed
the
gift-cards/add-recipient
branch
from
6eaad57
to
149b323
Compare
assets/customer.css
Outdated
| .customer td .properties { | ||
| text-align: left; | ||
| } |
There was a problem hiding this comment.
This looks odd to me 🤔 Even though aligned right is a bit odd as well I think it fits better with the rest of the layout:
Screenshot
cc: @danielvan could we get your input on this ?
There was a problem hiding this comment.
I know this one is hard. You can ping @YoannJailin if needed.
I agree right aligned is best, but right now the stacking is weird. Could we force a line break? E.g.
Recipient email:
Ludo.segura
Recipient name:
Lu
Message:
Testing this out
We can't design a table or something custom here for gift cards, right?
There was a problem hiding this comment.
Removed and left it aligned right for now. We can revisit this later 👍
| onChange() { | ||
| if (!this.checkboxInput.checked) { | ||
| this.clearInputFields(); | ||
| this.clearErrorMessage(); | ||
| } | ||
| } |
There was a problem hiding this comment.
For the accessibility part maybe in here we could add / remove content in a visually hidden element with a status role 🤔
This way when checked there can be a announcement that a form has appeared on the screen.
There was a problem hiding this comment.
Not something to deal with in this PR but a follow up for us to potentially look into is the font size usage. It seem to be a bit all over the place. cc: @danielvan
Screenshot
There was a problem hiding this comment.
Yes, @ludoboludo. Thanks for this example. I'm working on a type standardization brief. For this one, the only lever we have would be to adjust the size of the denomination? But I guess this comes from the variant settings.
| "form": { | ||
| "checkbox": "I want to send this as a gift", | ||
| "email_label": "Recipient email", | ||
| "email_label_optional": "Recipient email (optional)", |
There was a problem hiding this comment.
Nitpick to maybe keep as follow up so we do not mess with translations now: this is meant for the no-js behaviour if I remember correctly.
It's not very intuitive for the merchant though if they were editing their language file:
Similar comment for name and message since you don't see them on the page.
If i'm changing my wording then I'm not sure what corresponds to what.
There was a problem hiding this comment.
Yeah I'm not especially happy with how we mark things optional, I think it should be a badge or some other cue rather than part of the text. Similarly our use of placeholders is confusing, they are not used like traditional placeholders because we embed the label inside the field instead. The entry you have highlighted is indeed for the no-js case.
Agree there's not much we can do now but maybe an area for ux to have a rethink on.
| }, | ||
| "show_gift_card_recipient": { | ||
| "label": "Show recipient form for gift card products", | ||
| "info": "When enabled, gift card products can optionally be sent to a recipient with a personal message." |
There was a problem hiding this comment.
I think ideally here we would link to our help doc on how to add a gift card product: https://help.shopify.com/en/manual/products/gift-card-products/add-update-gift-card-products
Though doing it now means re requesting translations.
There was a problem hiding this comment.
Also might be good to mention that the dynamic checkout buttons will be hidden when this is showing 🤔
Co-authored-by: Ludo <ludo.segura@shopify.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
| .recipient-form .icon-checkmark { | ||
| visibility: hidden; | ||
| position: absolute; | ||
| left: 0.28rem; | ||
| z-index: 5; | ||
| top: 0.4rem; | ||
| } |
There was a problem hiding this comment.
I updated the checkmark 👍
| @@ -82,8 +82,8 @@ | |||
| {%- for property in line_item.properties -%} | |||
| {% assign property_first_char = property.first | slice: 0 %} | |||
| {%- if property.last != blank and property_first_char != '_' -%} | |||
| <span>{{ property.first }}:</span> | |||
There was a problem hiding this comment.
Added this so it's easier to differentiate the first and last property.
|
I created a follow up issue with some of the things that might not/won't be fixed before merging: #2454 |
* shopify/main: Gift cards/add recipient (Shopify#2412) Update 1 translation file (Shopify#2453) [Slideshow] Add ambient movement to background (Shopify#2383) Wrap calls to search results ind collection counts in paginate to reduce additional requests (Shopify#2421) [Header] Add app block in the header section (Shopify#2238) [Image with Text] Add ambient movement to image (Shopify#2385) Update 1 translation file (Shopify#2450) [Blog post section] Fix slides size on mobile (Shopify#2368) Duplicated scrollbar in drawer menu (Shopify#2400) [Header locales] Support gradients (Shopify#2386) [Image banner] Add ambient movement to background (Shopify#2342)
| rows="10" | ||
| id="Recipient-message-{{ section.id }}" | ||
| class="text-area field__input" | ||
| name="properties[Message]" |
There was a problem hiding this comment.
Can name="properties[Message]" be made consistent with the other properties so :
name="properties[Recipient email]"name="properties[Recipient name]"name="properties[Recipient message]"
properties[Message] can conflict with things like:
- css attribute rules(
[name="properties[Message]"]) - liquid or javascript operating on the "message" keyword in line items properties
- personalization apps
- And of course the confusion with message property of the line_item itself for discount messages.
Seemingly inconsistent namespacing is a bugbear in shopify that always causes headaches. I'd guess this case is a domino effect of trying to shape recipient like a permanent account rather than a single transactional entity for the purchase? Which is confusing because message should only exist if the recipient does. Instead right now it's on the gift-card object which then bleeds through to the frontend DX as the inconsistent properties[Message].
At minimum being aliased 🤷 within the recipient object for a gift card would be saner.
Also nickname seems to be missing as well mabye because that starts creating clutter or a confusing UI or something.
https://shopify.dev/docs/api/liquid/objects/recipient#recipient-nickname
There was a problem hiding this comment.
Lots of interesting feedback in here, thanks.
The main issue is simply that line item properties have to be both customer facing and machine intelligible so UX has an impact on something that should be an internal implementation detail. The inconsistency you highlighted in message naming was a specific request from UX.
The model for a recipient is just a customer object, we had a choice between saying the nickname and message belong to the customer or the gift card. We decided that we preferred the new fields on the gift card model because the nickname isn't necessarily something a customer wants associated with them forever.
Also note that what is referred to as a nickname in the liquid drop is just presented as Recipient Name in the FE. This is because we want to make a distinction on the back end between the actual customer.name provided by the customer themselves and the nickname which is provided by the sender.
As these changes went out in the last Dawn release we have to support the existing names indefinitely but I'd love see a future improvement to line item property to support localisation and general decoupling of key from display text.
There was a problem hiding this comment.
It occurred to me message being on the gift card||item is a bit more flexible in small chance of a future multiple recipients but same message feature.
..we preferred the new fields on the gift card model because the nickname isn't necessarily something a customer wants associated with them forever
In current liquid docs the nickname not the giftcard it is on the recipient . If the object<>props relations are different in other apis let me know as the graphiql,etc api docs do not seem to expose a recipient as yet.
The inconsistency you highlighted in message naming was a specific request from UX.
Perspective - LIP's are heavily used for merchant's arbitrary custom data coming from the FE, and now shopify is creating reserved namespaces with generic words in that merchant space for presentation purposes because of existing flaws. And undocumented reserved namespacing is the meta issue of: If you know you know, if you don't you don't
I get that for 1 or 2 here and there but if this grows the LIP approach is a problem and totally agree with you that it needs to be decoupled.
As these changes went out in the last Dawn release we have to support the existing names indefinitely but I'd love see a future improvement to line item property to support localisation and general decoupling of key from display text.
Really it seems like LIP's is not where this info should live and LIP's got used because it was available and perceived as free real estate. But ugh even as I type this I realize we cant' win because adding something like gift_card_attributes|gift_attributes just grows the rest api.
Thank you for the explanation, no need to reply.
There was a problem hiding this comment.
Great write up, you've packed our whole journey with LIPs into a succinct few paragraphs, all the way from "LIPs are a perfect fit for our feature" to "Oh no".
Just wanted to address one point
shopify is creating reserved namespaces with generic words in that merchant space for presentation purposes because of existing flaws
We do have a guard for this, our feature has to be activated using another "hidden" line item property which is __shopify_send_gift_card_to_recipient so for most cases it's still safe for a merchant to use the same generic LIP keys as us without risk. Of course there will be edges where our feature is active at the same time as some other feature which has collisions so the correct solution is still to decouple key and display value.
PR Summary:
Adds gift card add a recipient feature to Dawn theme.
This adds a new option on the buy buttons block which enables an extra set of form fields for collecting recipient information. The gift card is then sent to both the original customer and the intended recipient.
Why are these changes introduced?
It's a very common merchant frustration that we don't support this functionality for our gift card products.
What approach did you take?
It was decided early on that as gift cards are a native Shopify feature the add a recipient feature should also be native to our platform, hence it's implemented in Dawn rather than as a 1st party app.
Other considerations
Decision log
Visual impact on existing themes
Should have no impact until users explicitly enable the option on the buy buttons block at which point the new ui is shown.
Testing steps/scenarios
The beta flag
gift_card_add_recipientmust be set for anything good to happen currently. This flag will be rolled out 100% ahead of the release.Demo links
Checklist