Skip to content
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

Open
wants to merge 9 commits into
base: MWPW-164492
Choose a base branch
from

Conversation

bozojovicic
Copy link
Contributor

Add quantity selector and badge to plans cards. Quantity selector also had one bug: in text field you could set the value smaller than the minimal configured value.

Resolves: MWPW-167306

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Feb 13, 2025

Page Scores Audits Google
📱 /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

stockOffer: true,
secureLabel: true,
badge: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Comment on lines +126 to +129
function parseHtml(html) {
const parser = new DOMParser();
return parser.parseFromString(html, 'text/html');
}
Copy link
Contributor

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 within isHtmlEmpty itself, it will technical save 3 lines of code.

Comment on lines +98 to +103
let adjustedInputValue =
this.maxInput && inputValue > this.maxInput
? this.maxInput
: inputValue;
adjustedInputValue = this.min && adjustedInputValue < this.min
? this.min : adjustedInputValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let adjustedInputValue =
this.maxInput && inputValue > this.maxInput
? this.maxInput
: inputValue;
adjustedInputValue = this.min && adjustedInputValue < this.min
? this.min : adjustedInputValue;
let adjustedInputValue = inputValue;
if (this.maxInput && inputValue > this.maxInput)
adjustedInputValue = this.maxInput;
if (this.min && adjustedInputValue < this.min)
adjustedInputValue = this.min;

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.

@@ -189,7 +189,7 @@ runTests(async () => {
});
event.composedPath = () => [quantitySelect];
inputField.dispatchEvent(event);
await delay();
await delay(600);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@3ch023 3ch023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bozojovicic ,
could you pls also add a card to https://mwpw-167306--milo--bozojovicic.aem.page/libs/features/mas/docs/plans.html?
You need to clone an existing card in Nala folder in studio (important to be a Nala folder!) and then you can reference it in plans.md and run 'npm run build:docs' in /libs/mas/features - this will generate a new plans.html

and then could you pls add a test to masacom.test.js to verify quantity selector presense on that card and badge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants