MWPW-189463 MAS Studio: Quantity selector variation#700
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
Commits
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
+ Coverage 87.51% 87.55% +0.04%
==========================================
Files 214 216 +2
Lines 64039 64177 +138
==========================================
+ Hits 56042 56191 +149
+ Misses 7997 7986 -11
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@adobecom/loot-loaders reviews plz ! |
yesil
left a comment
There was a problem hiding this comment.
@bozojovicic why global settings are not being used for quantity select? I think we should avoid setting it at card level.
and, this setting should go to "Options and settings" as a setting override.
also comment on the code:
Code Review
1. Duplicate .field-status-indicator CSS
The same styles are now defined in both quantity-select.js and merch-card-editor.js. Consider extracting to a shared CSS module to avoid drift.
2. Sentinel value QUANTITY_EMPTY is fragile
const QUANTITY_EMPTY = '</merch-quantity-select>';Storing a closing HTML tag as a field value to signal "disabled" is unconventional and could confuse future developers or break if anything tries to parse/render it.
3. renderQuantityComponentOverrideIndicator does two different things
Without an argument it checks if the entire QS is overridden; with one it checks a sub-field. The startsWith('<merch-quantity-select ') comparison is also brittle if serialization format ever changes.
4. Manual requestUpdate() needs a comment
if (!e.target.checked) this.requestUpdate();This suggests a reactive property change isn't being detected by Lit. Worth a short comment explaining why it's needed.
|
@Roycethan I also fixed the broken Nala tests for the Quantity selector. In settings for Nala I configured to show the quantity selector for Education cards. |
|
@bozojovicic please resolve conflicts. |
Roycethan
left a comment
There was a problem hiding this comment.
@bozojovicic
Plz resolve conflicts
Resolves https://jira.corp.adobe.com/browse/MWPW-189463
QA Checklist: https://wiki.corp.adobe.com/display/adobedotcom/M@S+Engineering+QA+Use+Cases
QS values can now be defined in settings, enabled or disabled
if QS values are defined in settings and if QS is supported in the code for that card variant
we will see it in "Options and settings" section, either enabled or disabled
then we can enable/disable it for that card, redefine values, restore them, per one field or all values
the same for card variations
the same for translated cards
Variation https://mwpw-189463--mas--adobecom.aem.live/studio.html#fragmentId=f8b64edc-23a9-4b54-bc78-72ff67b97aba&page=fragment-editor&path=sandbox
Parent https://mwpw-189463--mas--adobecom.aem.live/studio.html#fragmentId=8ac4f953-ce06-4792-8390-8d00c89d689b&page=fragment-editor&path=sandbox
Localized https://mwpw-189463--mas--adobecom.aem.live/studio.html#fragmentId=47595fac-7758-4fa8-83b1-45578eabf758&locale=fr_FR&page=fragment-editor&path=sandbox&tags=mas%3Avariant%2Fplans-education
Milo pages https://main--milo--adobecom.aem.page/drafts/bozo/pr/qs-settings?maslibs=mwpw-189463
https://main--milo--adobecom.aem.page/be_en/drafts/bozo/qs?maslibs=MWPW-189463
https://main--milo--adobecom.aem.page/fr/drafts/bozo/qs?maslibs=MWPW-189463
Settings for Quantity select https://mas.adobe.com/studio.html#fragmentId=783d1952-fff2-481c-8a26-9461de41f3fa&page=settings-editor&path=sandbox
Please do the steps below before submitting your PR for a code review or QA
🧪 Nala E2E Tests
Nala tests run automatically when you open this PR.
To run Nala tests again:
run nalalabel to this PR (in the right sidebar)To stop automatic Nala tests:
run nalalabelTest URLs: