-
Notifications
You must be signed in to change notification settings - Fork 180
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
MWPW-167306 [Plans Milo] Quantity Selector & Badge #3683
base: MWPW-164492
Are you sure you want to change the base?
Changes from all commits
0052640
f6fd4a3
99cd440
a344412
0789e87
f85be67
627e472
07a5b7e
c091a9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||||||||||||||||||||
import { html, LitElement } from 'lit'; | ||||||||||||||||||||||||
import { styles } from './merch-quantity-select.css.js'; | ||||||||||||||||||||||||
import { debounce } from './utils.js'; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
import { ARROW_DOWN, ARROW_UP, ENTER } from './focus.js'; | ||||||||||||||||||||||||
import { EVENT_MERCH_QUANTITY_SELECTOR_CHANGE } from './constants.js'; | ||||||||||||||||||||||||
|
@@ -43,6 +44,7 @@ export class MerchQuantitySelect extends LitElement { | |||||||||||||||||||||||
this.boundKeydownListener = this.handleKeydown.bind(this); | ||||||||||||||||||||||||
this.addEventListener('keydown', this.boundKeydownListener); | ||||||||||||||||||||||||
window.addEventListener('mousedown', this.handleClickOutside); | ||||||||||||||||||||||||
this.handleKeyupDebounced = debounce(this.handleKeyup.bind(this), 500); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
handleKeyup() { | ||||||||||||||||||||||||
|
@@ -93,10 +95,12 @@ export class MerchQuantitySelect extends LitElement { | |||||||||||||||||||||||
inputValue > 0 && | ||||||||||||||||||||||||
inputValue !== this.selectedValue | ||||||||||||||||||||||||
) { | ||||||||||||||||||||||||
const adjustedInputValue = | ||||||||||||||||||||||||
let adjustedInputValue = | ||||||||||||||||||||||||
this.maxInput && inputValue > this.maxInput | ||||||||||||||||||||||||
? this.maxInput | ||||||||||||||||||||||||
: inputValue; | ||||||||||||||||||||||||
adjustedInputValue = this.min && adjustedInputValue < this.min | ||||||||||||||||||||||||
? this.min : adjustedInputValue; | ||||||||||||||||||||||||
Comment on lines
+98
to
+103
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.
Suggested change
I think this makes it a bit more readable, I found the reassignments and ternaries to be a bit hard to read with the compound conditionals ahead of them. Let me know what you think. |
||||||||||||||||||||||||
this.selectedValue = adjustedInputValue; | ||||||||||||||||||||||||
inputField.value = adjustedInputValue; | ||||||||||||||||||||||||
this.highlightedIndex = this.options.indexOf(adjustedInputValue); | ||||||||||||||||||||||||
|
@@ -210,7 +214,7 @@ export class MerchQuantitySelect extends LitElement { | |||||||||||||||||||||||
.value="${this.selectedValue}" | ||||||||||||||||||||||||
type="number" | ||||||||||||||||||||||||
@keydown="${this.handleKeydown}" | ||||||||||||||||||||||||
@keyup="${this.handleKeyup}" | ||||||||||||||||||||||||
@keyup="${this.handleKeyupDebounced}" | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
<button class="picker-button" @click="${this.toggleMenu}"> | ||||||||||||||||||||||||
<div | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,10 @@ export const PLANS_AEM_FRAGMENT_MAPPING = { | |
description: { tag: 'div', slot: 'body-xs' }, | ||
mnemonics: { size: 'l' }, | ||
callout: {tag: 'div', slot: 'callout-content'}, | ||
quantitySelect: { tag: 'div', slot: 'quantity-select' }, | ||
stockOffer: true, | ||
secureLabel: true, | ||
badge: true, | ||
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. why this is needed? 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. Card editor reads this mapping to decide which authoring fields will be displayed for 'plans' cards. For card badge (the yellow thing in the upper right corner) we don't have slots but it needs to be mentioned here to display that Badge text field in card editor. The same as for stock checkbox. |
||
ctas: { slot: 'footer', size: 'm' }, | ||
style: 'consonant' | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ runTests(async () => { | |
}); | ||
event.composedPath = () => [quantitySelect]; | ||
inputField.dispatchEvent(event); | ||
await delay(); | ||
await delay(600); | ||
expect(quantitySelect.selectedValue).to.equal(3); | ||
expect(popOver.classList.contains('closed')).to.be.true; | ||
}); | ||
|
@@ -155,7 +155,7 @@ runTests(async () => { | |
}); | ||
event.composedPath = () => [quantitySelect]; | ||
inputField.dispatchEvent(event); | ||
await delay(); | ||
await delay(600); | ||
expect(quantitySelect.selectedValue).to.equal(3); | ||
expect(popOver.classList.contains('closed')).to.be.true; | ||
}); | ||
|
@@ -189,7 +189,7 @@ runTests(async () => { | |
}); | ||
event.composedPath = () => [quantitySelect]; | ||
inputField.dispatchEvent(event); | ||
await delay(); | ||
await delay(600); | ||
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. In regards to all of these, is 600 the minimum ms you can delay? If i am reading this correctly, this will add 2.4s to our testing suite. |
||
expect(quantitySelect.selectedValue).to.equal(250); | ||
}); | ||
}); | ||
|
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 dont see this getting reused, and only called in
isHtmlEmpty
. Is there a reason not to instatiate the parser and parse withinisHtmlEmpty
itself, it will technical save 3 lines of code.