-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gift cards/add recipient #2412
Gift cards/add recipient #2412
Changes from 65 commits
c49fbe9
e61dbff
bc5dead
e3f0cb5
89d3e65
41467d2
fa1d844
a7884a1
64488ef
247a4e9
e9380e0
b9f5486
1e9763f
49d19d5
793406a
1bb0d96
617fc7f
f92b3d4
16f7fb2
70040ec
84d8e85
46e9e2b
517d99c
57cb5a7
053cf8e
d624c4b
701e107
73a7cd3
ec25e91
fbcb62f
17defb7
ab74e9b
c29e433
a8cea7f
aa7b4e5
e94ecf0
cd8f958
b61ae4a
9543809
722bfee
4a2a92f
e698fc0
c251a19
091373d
b470d20
f9f50d6
b656d7a
fe869bb
6aa7f56
f753c47
2a81400
d30acab
ff95e95
e562f4e
fd30c0d
405e665
92380a6
69b07ee
7cd7e6c
5fd63f7
f1a3246
c9b9283
e192755
149b323
0204dfe
8e29538
f578763
e720fba
d305bac
4cecb78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
if (!customElements.get('recipient-form')) { | ||
customElements.define('recipient-form', class RecipientForm extends HTMLElement { | ||
constructor() { | ||
super(); | ||
this.checkboxInput = this.querySelector(`#Recipient-Checkbox-${ this.dataset.sectionId }`); | ||
this.checkboxInput.disabled = false; | ||
this.hiddenControlField = this.querySelector(`#Recipient-Control-${ this.dataset.sectionId }`); | ||
this.hiddenControlField.disabled = true; | ||
this.emailInput = this.querySelector(`#Recipient-email-${ this.dataset.sectionId }`); | ||
this.nameInput = this.querySelector(`#Recipient-name-${ this.dataset.sectionId }`); | ||
this.messageInput = this.querySelector(`#Recipient-message-${ this.dataset.sectionId }`); | ||
this.errorMessageWrapper = this.querySelector('.product-form__recipient-error-message-wrapper'); | ||
this.errorMessageList = this.errorMessageWrapper?.querySelector('ul'); | ||
this.errorMessage = this.errorMessageWrapper?.querySelector('.error-message'); | ||
this.defaultErrorHeader = this.errorMessage?.innerText; | ||
this.currentProductVariantId = this.dataset.productVariantId; | ||
this.addEventListener('change', this.onChange.bind(this)); | ||
} | ||
|
||
cartUpdateUnsubscriber = undefined; | ||
variantChangeUnsubscriber = undefined; | ||
cartErrorUnsubscriber = undefined; | ||
|
||
connectedCallback() { | ||
this.cartUpdateUnsubscriber = subscribe(PUB_SUB_EVENTS.cartUpdate, (event) => { | ||
if (event.source === 'product-form' && event.productVariantId.toString() === this.currentProductVariantId) { | ||
this.resetRecipientForm(); | ||
} | ||
}); | ||
|
||
this.variantChangeUnsubscriber = subscribe(PUB_SUB_EVENTS.variantChange, (event) => { | ||
if (event.data.sectionId === this.dataset.sectionId) { | ||
this.currentProductVariantId = event.data.variant.id.toString(); | ||
} | ||
}); | ||
|
||
this.cartUpdateUnsubscriber = subscribe(PUB_SUB_EVENTS.cartError, (event) => { | ||
if (event.source === 'product-form' && event.productVariantId.toString() === this.currentProductVariantId) { | ||
this.displayErrorMessage(event.message, event.errors); | ||
} | ||
}); | ||
} | ||
|
||
disconnectedCallback() { | ||
if (this.cartUpdateUnsubscriber) { | ||
this.cartUpdateUnsubscriber(); | ||
} | ||
|
||
if (this.variantChangeUnsubscriber) { | ||
this.variantChangeUnsubscriber(); | ||
} | ||
|
||
if (this.cartErrorUnsubscriber) { | ||
this.cartErrorUnsubscriber(); | ||
} | ||
} | ||
|
||
onChange() { | ||
if (!this.checkboxInput.checked) { | ||
this.clearInputFields(); | ||
this.clearErrorMessage(); | ||
} | ||
} | ||
Comment on lines
+58
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the accessibility part maybe in here we could add / remove content in a visually hidden element with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #2672 |
||
|
||
clearInputFields() { | ||
this.emailInput.value = ''; | ||
this.nameInput.value = ''; | ||
this.messageInput.value = ''; | ||
} | ||
|
||
displayErrorMessage(title, body) { | ||
this.clearErrorMessage(); | ||
this.errorMessageWrapper.hidden = false; | ||
if (typeof body === 'object') { | ||
this.errorMessage.innerText = this.defaultErrorHeader; | ||
return Object.entries(body).forEach(([key, value]) => { | ||
const errorMessageId = `RecipientForm-${ key }-error-${ this.dataset.sectionId }` | ||
const fieldSelector = `#Recipient-${ key }-${ this.dataset.sectionId }`; | ||
const placeholderElement = this.querySelector(`${fieldSelector}`); | ||
const label = placeholderElement?.getAttribute('placeholder') || key; | ||
const message = `${label} ${value}`; | ||
const errorMessageElement = this.querySelector(`#${errorMessageId}`); | ||
const errorTextElement = errorMessageElement?.querySelector('.error-message') | ||
if (!errorTextElement) return; | ||
|
||
if (this.errorMessageList) { | ||
this.errorMessageList.appendChild(this.createErrorListItem(fieldSelector, message)); | ||
} | ||
|
||
errorTextElement.innerText = `${message}.`; | ||
errorMessageElement.classList.remove('hidden'); | ||
|
||
const inputElement = this[`${key}Input`]; | ||
if (!inputElement) return; | ||
|
||
inputElement.setAttribute('aria-invalid', true); | ||
inputElement.setAttribute('aria-describedby', errorMessageId); | ||
}); | ||
} | ||
|
||
this.errorMessage.innerText = body; | ||
} | ||
|
||
createErrorListItem(target, message) { | ||
const li = document.createElement('li'); | ||
const a = document.createElement('a'); | ||
a.setAttribute('href', target); | ||
a.innerText = message; | ||
li.appendChild(a); | ||
li.className = "error-message"; | ||
return li; | ||
} | ||
|
||
clearErrorMessage() { | ||
this.errorMessageWrapper.hidden = true; | ||
|
||
if (this.errorMessageList) this.errorMessageList.innerHTML = ''; | ||
|
||
this.querySelectorAll('.recipient-fields .form__message').forEach(field => { | ||
field.classList.add('hidden'); | ||
const textField = field.querySelector('.error-message'); | ||
if (textField) textField.innerText = ''; | ||
}); | ||
|
||
[this.emailInput, this.messageInput, this.nameInput].forEach(inputElement => { | ||
inputElement.setAttribute('aria-invalid', false); | ||
inputElement.removeAttribute('aria-describedby'); | ||
}); | ||
} | ||
|
||
resetRecipientForm() { | ||
if (this.checkboxInput.checked) { | ||
this.checkboxInput.checked = false; | ||
this.clearInputFields(); | ||
this.clearErrorMessage(); | ||
} | ||
} | ||
}); | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,7 +161,7 @@ | |
flex: 0 0 100%; | ||
padding: 0; | ||
margin: 0 0 1.2rem 0; | ||
max-width: 37rem; | ||
max-width: 44rem; | ||
antiBaconMachine marked this conversation as resolved.
Show resolved
Hide resolved
|
||
min-width: fit-content; | ||
border: none; | ||
} | ||
|
@@ -1473,3 +1473,129 @@ a.product__text { | |
display: none; | ||
} | ||
} | ||
|
||
/* Recipient form */ | ||
.recipient-form { | ||
/* (2.88[line-height] - 1.6rem) / 2 */ | ||
--recipient-checkbox-margin-top: 0.64rem; | ||
|
||
display: block; | ||
position: relative; | ||
max-width: 44rem; | ||
margin-bottom: 2.5rem; | ||
} | ||
|
||
.recipient-form-field-label { | ||
margin: 0.6rem 0; | ||
} | ||
|
||
.recipient-form-field-label--space-between { | ||
display: flex; | ||
justify-content: space-between; | ||
} | ||
|
||
.recipient-checkbox { | ||
flex-grow: 1; | ||
font-size: 1.6rem; | ||
display: flex; | ||
word-break: break-word; | ||
align-items: flex-start; | ||
max-width: inherit; | ||
} | ||
|
||
.no-js .recipient-checkbox { | ||
display: none; | ||
} | ||
|
||
.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; | ||
} | ||
Comment on lines
+1511
to
+1521
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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-fields__field { | ||
margin: 0 0 2rem 0; | ||
} | ||
|
||
.recipient-fields .field__label { | ||
white-space: nowrap; | ||
text-overflow: ellipsis; | ||
max-width: calc(100% - 3.5rem); | ||
overflow: hidden; | ||
} | ||
|
||
.recipient-checkbox > svg { | ||
margin-top: var(--recipient-checkbox-margin-top); | ||
margin-right: 1.2rem; | ||
flex-shrink: 0; | ||
} | ||
|
||
.recipient-form .icon-checkmark { | ||
visibility: hidden; | ||
position: absolute; | ||
left: 0.35rem; | ||
z-index: 5; | ||
top: 0.45rem; | ||
height: 0.7rem; | ||
} | ||
|
||
.recipient-form > input[type='checkbox']:checked + label .icon-checkmark { | ||
visibility: visible; | ||
} | ||
|
||
.js .recipient-fields { | ||
display: none; | ||
} | ||
|
||
.recipient-fields hr { | ||
margin: 1.6rem auto; | ||
} | ||
|
||
.recipient-form > input[type='checkbox']:checked ~ .recipient-fields { | ||
display: block; | ||
animation: animateMenuOpen var(--duration-default) ease; | ||
} | ||
Comment on lines
+1560
to
+1563
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same feeling on my side. I brought the possibility to add something like toggling an attribute There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something on which we could get @metamoni's take 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I did some quick experiments with aria-expanded and concluded that it was the wrong tool for the job. |
||
.recipient-form > input[type='checkbox']:not(:checked, :disabled) ~ .recipient-fields , | ||
.recipient-email-label { | ||
display: none; | ||
} | ||
|
||
.js .recipient-email-label.required, | ||
.no-js .recipient-email-label.optional { | ||
display: inline; | ||
} | ||
|
||
.recipient-form ul { | ||
line-height: calc(1 + 0.6 / var(--font-body-scale)); | ||
padding-left: 4.4rem; | ||
text-align: left; | ||
} | ||
|
||
.recipient-form ul a { | ||
display: inline; | ||
} | ||
|
||
.recipient-form .error-message::first-letter { | ||
text-transform: capitalize; | ||
} | ||
|
||
@media screen and (forced-colors: active) { | ||
.recipient-fields > hr { | ||
border-top: 0.1rem solid rgb(var(--color-background)); | ||
} | ||
|
||
.recipient-checkbox > svg { | ||
background-color: inherit; | ||
border: 0.1rem solid rgb(var(--color-background)); | ||
} | ||
|
||
.recipient-form > input[type='checkbox']:checked + label .icon-checkmark { | ||
border: none; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and left it aligned right for now. We can revisit this later 👍