-
Notifications
You must be signed in to change notification settings - Fork 177
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-155057: Lets authors add "starting at" to merch cards #2717
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2717 +/- ##
==========================================
- Coverage 95.91% 95.91% -0.01%
==========================================
Files 173 173
Lines 45816 45831 +15
==========================================
+ Hits 43943 43957 +14
- Misses 1873 1874 +1 ☔ View full report in Codecov by Sentry. |
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
Hey @Axelcureno - I'm not able to see the "starting at" text in your demo page https://mwpw-155057--milo--adobecom.hlx.page/drafts/axel/stories/mwpw-155057?martech=off |
@Axelcureno Should we add some unit tests for this? |
@narcis-radu that's strange, I still see the change. Maybe try clearing cache? |
@Axelcureno I guess we probably should update the authoring documentation after this change is live |
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.
Just a nit for the description:
Content is inserted using placeholder {{starting-as}}.
has a small typo, should be {{starting-at}}
, just to avoid any confusion
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.
nit request
libs/blocks/merch-card/merch-card.js
Outdated
const { replaceKey } = await import('../../features/placeholders.js'); | ||
await replaceKey('starting-at', getConfig()).then((key) => { | ||
const startingAt = createTag('div', { slot: 'starting-at' }, key); | ||
const prices = merchCard.querySelectorAll('span[is="inline-price"]'); |
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.
please rather use querySelector
if you are just going to use the first one
@Axelcureno your changes are adding/exposing Could you also update the branch with the stage latest and make sure you don't have stale block so that we can validate the PR? This issue has been seen in the recent Stage to Main PRs. https://main--cc--adobecom.hlx.live/pl/creativecloud?milolibs=mwpw-155057 |
@afmicka It wasn't my changes that were adding the |
Skipped 2717: "MWPW-155057: Lets authors add "starting at" to merch cards" due to file "libs/deps/mas/merch-card-all.js" overlap. Merging will be attempted in the next batch |
Skipped 2717: "MWPW-155057: Lets authors add "starting at" to merch cards" due to file "libs/deps/mas/merch-card-all.js" overlap. Merging will be attempted in the next batch |
Lets authors add "Starting at" to merch card content before the price.
Supported merch card variants:
Resolves: MWPW-155057
Test URLs: