-
Notifications
You must be signed in to change notification settings - Fork 168
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-156612 fragment merch-card variant layouts #2810
Conversation
will probably need to wait for #2798 but reviews can start |
- move from merch-card.js code each layout code related to a given variant under variants/<variant> class - move from global.css.js css related to a given variant under variants/<variant>.css.js - move from merch-card.css.js wc css related to a given variant under variants/<variant>.variantStyle - uses variants/variants as an index to get related class from related variant, and wc style too, - uses variants/VariantLayout, superclass of all variants, to insert css if card is used, and hold common code
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. |
(i also need to check why TBT is increased) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2810 +/- ##
==========================================
+ Coverage 96.18% 96.22% +0.04%
==========================================
Files 215 237 +22
Lines 53969 54231 +262
==========================================
+ Hits 51909 52185 +276
+ Misses 2060 2046 -14 ☔ View full report in Codecov by Sentry. |
to something independent of the layout (not initialized when used at first in collections)
8b0488e
to
b9580d5
Compare
libs/deps/mas/merch-card.js
Outdated
<footer><slot name="footer"></slot></footer> | ||
${this.defaultSlot} | ||
</div>`}connectedCallback(){super.connectedCallback(),this.#e=this.getContainer(),this.setAttribute("tabindex",this.getAttribute("tabindex")??"0"),this.addEventListener("mouseleave",this.toggleActionMenu),this.addEventListener(k,this.handleQuantitySelection),this.addEventListener(_,this.merchCardReady,{once:!0}),this.updateComplete.then(()=>{this.merchCardReady()}),this.storageOptions?.addEventListener("change",this.handleStorageChange)}disconnectedCallback(){super.disconnectedCallback(),this.removeEventListener(k,this.handleQuantitySelection),this.storageOptions?.removeEventListener(C,this.handleStorageChange)}appendInvisibleSpacesToFooterLinks(){[...this.querySelectorAll('[slot="footer"] a')].forEach(e=>{L(e).forEach(r=>{let i=r.textContent.split(" ").map(u=>u.match(/.{1,7}/g)?.join("\u200B")).join(" ");r.textContent=i})})}updateMiniCompareElementMinHeight(e,t){let r=`--consonant-merch-card-mini-compare-${t}-height`,n=Math.max(0,parseInt(window.getComputedStyle(e).height)||0),a=parseInt(this.#e.style.getPropertyValue(r))||0;n>a&&this.#e.style.setProperty(r,`${n}px`)}async adjustTitleWidth(){if(!["segment","plans"].includes(this.variant))return;let e=this.getBoundingClientRect().width,t=this.badgeElement?.getBoundingClientRect().width||0;e===0||t===0||this.style.setProperty("--consonant-merch-card-heading-xs-max-width",`${Math.round(e-t-16)}px`)}async adjustMiniCompareBodySlots(){if(this.variant!==E||this.getBoundingClientRect().width===0)return;this.updateMiniCompareElementMinHeight(this.shadowRoot.querySelector(".top-section"),"top-section"),["heading-m","body-m","heading-m-price","price-commitment","offers","promo-text","callout-content","secure-transaction-label"].forEach(r=>this.updateMiniCompareElementMinHeight(this.shadowRoot.querySelector(`slot[name="${r}"]`),r)),this.updateMiniCompareElementMinHeight(this.shadowRoot.querySelector("footer"),"footer");let t=this.shadowRoot.querySelector(".mini-compare-chart-badge");t&&t.textContent!==""&&this.#e.style.setProperty("--consonant-merch-card-mini-compare-top-section-mobile-height","32px")}adjustMiniCompareFooterRows(){if(this.variant!==E||this.getBoundingClientRect().width===0)return;[...this.querySelector('[slot="footer-rows"]').children].forEach((t,r)=>{let n=Math.max(Z,parseInt(window.getComputedStyle(t).height)||0),a=parseInt(this.#e.style.getPropertyValue(j(r+1)))||0;n>a&&this.#e.style.setProperty(j(r+1),`${n}px`)})}removeEmptyRows(){if(this.variant!==E)return;this.querySelectorAll(".footer-row-cell").forEach(t=>{let r=t.querySelector(".footer-row-cell-description");r&&!r.textContent.trim()&&t.remove()})}get storageOptions(){return this.querySelector("sp-radio-group#storage")}get storageSpecificOfferSelect(){let e=this.storageOptions?.selected;if(e){let t=this.querySelector(`merch-offer-select[storage="${e}"]`);if(t)return t}return this.querySelector("merch-offer-select")}get offerSelect(){return this.storageOptions?this.storageSpecificOfferSelect:this.querySelector("merch-offer-select")}get quantitySelect(){return this.querySelector("merch-quantity-select")}merchCardReady(){this.offerSelect&&!this.offerSelect.planType||this.dispatchEvent(new CustomEvent(M,{bubbles:!0}))}handleStorageChange(){let e=this.closest("merch-card")?.offerSelect.cloneNode(!0);e&&this.dispatchEvent(new CustomEvent(C,{detail:{offerSelect:e},bubbles:!0}))}get dynamicPrice(){return this.querySelector('[slot="price"]')}selectMerchOffer(e){if(e===this.merchOffer)return;this.merchOffer=e;let t=this.dynamicPrice;if(e.price&&t){let r=e.price.cloneNode(!0);t.onceSettled?t.onceSettled().then(()=>{t.replaceWith(r)}):t.replaceWith(r)}}};customElements.define(g,p); | ||
`;document.head.appendChild(bt);var s="merch-card",i=class extends Nt{static properties={name:{type:String,attribute:"name",reflect:!0},variant:{type:String,reflect:!0},size:{type:String,attribute:"size",reflect:!0},badgeColor:{type:String,attribute:"badge-color"},borderColor:{type:String,attribute:"border-color"},badgeBackgroundColor:{type:String,attribute:"badge-background-color"},badgeText:{type:String,attribute:"badge-text"},actionMenu:{type:Boolean,attribute:"action-menu"},actionMenuContent:{type:String,attribute:"action-menu-content"},customHr:{type:Boolean,attribute:"custom-hr"},detailBg:{type:String,attribute:"detail-bg"},secureLabel:{type:String,attribute:"secure-label"},checkboxLabel:{type:String,attribute:"checkbox-label"},selected:{type:Boolean,attribute:"aria-selected",reflect:!0},storageOption:{type:String,attribute:"storage",reflect:!0},stockOfferOsis:{type:Object,attribute:"stock-offer-osis",converter:{fromAttribute:t=>{let[e,r,n]=t.split(",");return{PUF:e,ABM:r,M2M:n}}}},filters:{type:String,reflect:!0,converter:{fromAttribute:t=>Object.fromEntries(t.split(",").map(e=>{let[r,n,d]=e.split(":"),u=Number(n);return[r,{order:isNaN(u)?void 0:u,size:d}]})),toAttribute:t=>Object.entries(t).map(([e,{order:r,size:n}])=>[e,r,n].filter(d=>d!=null).join(":")).join(",")}},types:{type:String,attribute:"types",reflect:!0},merchOffer:{type:Object}};static styles=[ot,vt(),...nt()];customerSegment;marketSegment;variantLayout;constructor(){super(),this.filters={},this.types="",this.selected=!1}updated(t){(t.has("badgeBackgroundColor")||t.has("borderColor"))&&(this.style.border=this.computedBorderStyle),this.updateComplete.then(async()=>{let r=Array.from(this.querySelectorAll('span[is="inline-price"][data-wcs-osi]')).filter(n=>!n.closest('[slot="callout-content"]'));await Promise.all(r.map(n=>n.onceSettled())),this.variantLayout.postCardUpdateHook(this)})}render(){if(!(!this.isConnected||this.style.display==="none"))return this.variantLayout.renderLayout()}get computedBorderStyle(){return this.variant!=="twp"?`1px solid ${this.borderColor?this.borderColor:this.badgeBackgroundColor}`:""}get badgeElement(){return this.shadowRoot.getElementById("badge")}get headingmMSlot(){return this.shadowRoot.querySelector('slot[name="heading-m"]').assignedElements()[0]}get footerSlot(){return this.shadowRoot.querySelector('slot[name="footer"]')?.assignedElements()[0]}get price(){return this.headingmMSlot?.querySelector('span[is="inline-price"]')}get checkoutLinks(){return[...this.footerSlot?.querySelectorAll('a[is="checkout-link"]')??[]]}async toggleStockOffer({target:t}){if(!this.stockOfferOsis)return;let e=this.checkoutLinks;if(e.length!==0)for(let r of e){await r.onceSettled();let n=r.value?.[0]?.planType;if(!n)return;let d=this.stockOfferOsis[n];if(!d)return;let u=r.dataset.wcsOsi.split(",").filter(v=>v!==d);t.checked&&u.push(d),r.dataset.wcsOsi=u.join(",")}}handleQuantitySelection(t){let e=this.checkoutLinks;for(let r of e)r.dataset.quantity=t.detail.option}get titleElement(){return this.querySelector(".card-heading")}get title(){return this.titleElement?.textContent?.trim()}get description(){return this.querySelector('[slot="body-xs"]')?.textContent?.trim()}updateFilters(t){let e={...this.filters};Object.keys(e).forEach(r=>{if(t){e[r].order=Math.min(e[r].order||2,2);return}let n=e[r].order;n===1||isNaN(n)||(e[r].order=Number(n)+1)}),this.filters=e}includes(t){return this.textContent.match(new RegExp(t,"i"))!==null}get startingAt(){return this.classList.contains("starting-at")}get defaultSlot(){return this.querySelector(":scope > a:not([slot]),:scope > p:not([slot]),:scope > div:not([slot]),:scope > span:not([slot])")?Pt`<slot></slot>`:Dt}connectedCallback(){super.connectedCallback(),this.variantLayout=xt(this),this.variantLayout.connectedCallbackHook(),this.setAttribute("tabindex",this.getAttribute("tabindex")??"0"),this.addEventListener(N,this.handleQuantitySelection),this.addEventListener(W,this.merchCardReady,{once:!0}),this.updateComplete.then(()=>{this.merchCardReady()}),this.storageOptions?.addEventListener("change",this.handleStorageChange)}disconnectedCallback(){super.disconnectedCallback(),this.variantLayout.disconnectedCallbackHook(),this.removeEventListener(N,this.handleQuantitySelection),this.storageOptions?.removeEventListener(P,this.handleStorageChange)}appendInvisibleSpacesToFooterLinks(){[...this.querySelectorAll('[slot="footer"] a')].forEach(t=>{Z(t).forEach(r=>{let u=r.textContent.split(" ").map(v=>v.match(/.{1,7}/g)?.join("\u200B")).join(" ");r.textContent=u})})}get storageOptions(){return this.querySelector("sp-radio-group#storage")}get storageSpecificOfferSelect(){let t=this.storageOptions?.selected;if(t){let e=this.querySelector(`merch-offer-select[storage="${t}"]`);if(e)return e}return this.querySelector("merch-offer-select")}get offerSelect(){return this.storageOptions?this.storageSpecificOfferSelect:this.querySelector("merch-offer-select")}get quantitySelect(){return this.querySelector("merch-quantity-select")}merchCardReady(){this.offerSelect&&!this.offerSelect.planType||this.dispatchEvent(new CustomEvent(Y,{bubbles:!0}))}handleStorageChange(){let t=this.closest("merch-card")?.offerSelect.cloneNode(!0);t&&this.dispatchEvent(new CustomEvent(P,{detail:{offerSelect:t},bubbles:!0}))}get dynamicPrice(){return this.querySelector('[slot="price"]')}selectMerchOffer(t){if(t===this.merchOffer)return;this.merchOffer=t;let e=this.dynamicPrice;if(t.price&&e){let r=t.price.cloneNode(!0);e.onceSettled?e.onceSettled().then(()=>{e.replaceWith(r)}):e.replaceWith(r)}}};customElements.define(s,i); | ||
//# sourceMappingURL=merch-card.js.map |
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: let's remove the source maps please
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'll merge @yesil 's PR that does that, but yes
adjustMiniCompareFooterRows () { | ||
if (this.card.getBoundingClientRect().width === 0) return; | ||
const footerRows = this.card.querySelector('[slot="footer-rows"]'); | ||
[...footerRows.children].forEach((el, index) => { |
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: might be safer to use ...footerRows?.children
to avoid situations where the footer-rows
element is not available.
--consonant-merch-card-twp-mobile-height: 358px; | ||
} | ||
merch-card[variant="twp"] div[class$='twp-badge'] { | ||
padding: 4px 10px 5px 10px; |
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.
is RTL covered?
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.
not yet, no this variant is still under dev
grid-template-columns: var(--consonant-merch-card-image-width); | ||
} | ||
|
||
@media screen and ${DESKTOP_UP} { |
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 am looking at the order and I think declarations should follow mobile to large desktop or vice versa, however here we have desktop, large desktop, mobile, tablet, which might get confusing at some point when different CSS is added
|
||
insertVariantStyle() { | ||
if (!VariantLayout?.styleMap?.[this.card.variant]) { | ||
VariantLayout.styleMap[this.card.variant] = true; |
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 has potential to break; let's say VariantLayout.styleMap
is undefined, then your if clause will evaluate to true and it will execute VariantLayout.styleMap[this.card.variant] = true;
which will throw an error.
} | ||
|
||
postCardUpdateHook() { | ||
//nothing to do by default |
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.
are these methods still needed then?
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 is mother class VariantLayout, this method does not do anything by default but can be overriden (check MiniPlansCompareChart & Segment layout inheriting from it)
@@ -716,6 +297,28 @@ export class MerchCard extends LitElement { | |||
); | |||
} | |||
|
|||
toggleActionMenu(e) { |
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 method is only applicable to catalog
card. I wonder if it should be moved to catalog.js
?
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.
had some issues there but you are right, i'll try again :)
@media screen and ${DESKTOP_UP} { | ||
:root { | ||
--consonant-merch-card-catalog-width: 276px; | ||
} |
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: add missing line
@media screen and ${DESKTOP_UP} { | ||
:root { | ||
--consonant-merch-card-image-width: 378px; | ||
} |
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: add missing line
@media screen and ${DESKTOP_UP} { | ||
:root { | ||
--consonant-merch-card-product-width: 378px; | ||
} |
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: add missing line
@media screen and ${TABLET_UP} { | ||
:root { | ||
--consonant-merch-card-segment-width: 276px; | ||
} |
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: add missing line
@media screen and ${TABLET_UP} { | ||
:root { | ||
--consonant-merch-card-special-offers-width: 302px; | ||
} |
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: add missing line
static variantStyle = css` | ||
:host([variant='special-offers']) { | ||
min-height: 439px; | ||
} |
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: add missing line
--consonant-merch-card-twp-width: 268px; | ||
--consonant-merch-card-twp-mobile-width: 300px; | ||
--consonant-merch-card-twp-mobile-height: 358px; | ||
} |
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: add missing line
grid-template-columns: repeat(2, var(--consonant-merch-card-twp-width)); | ||
} | ||
.three-merch-cards.twp { | ||
grid-template-columns: repeat(3, var(--consonant-merch-card-twp-width)); |
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: indent line
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.
Some nits that could be ignored, but this one does need attention:
https://github.com/adobecom/milo/pull/2810/files#r1750613115
@npeltier the style of merch-card with background opacity has changed. Was that intentional? |
@npeltier Seeing also differences in width on mobile for special offers cards: |
@npeltier the changes are also affecting consumer pages:
|
8d29719
to
0c635a1
Compare
@afmicka background image, special offers, and bacom page should be fixed now. The width issue on kitchen sink is due to style ordering in case of multiple type of cards inserted in the same section. i'm not sure what we want here & started to discuss this internally. |
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.
Looks like more styles can be moved from global.css.js to the merch-card.css.js and some suggestion on the naming.
Overall it looks good to me, will wait for the comments before approval.
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.
all the rules like div[slot="footer"]
should be movable to merch-card.css.js
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.
sure, i thought we would wait for the move to "optimize" but i'll do that :)
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.
finally, same as for web tests, i'll postpone it as it's not that easy neither :)
import { html } from 'lit'; | ||
|
||
export class VariantLayout { | ||
static styleMap = {}; |
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: I tend to use module scoped variants instead of static members.
Especially if they are meant to be private
card; | ||
|
||
insertVariantStyle() { | ||
if (!VariantLayout.styleMap[this.card.variant]) { |
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: with above suggestion, it could be written as if (!styleMap[this.card.variant]) {
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.
since we append the style below, I would call the method appendVariantStyles
|
||
constructor(card) { | ||
this.card = card; | ||
setTimeout(() => this.insertVariantStyle(), 1); |
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.
is setTimeout
really needed?
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 think i did save some TBT with this yes
await mockFetch(withWcs); | ||
await mas(); | ||
if (skipTests !== null) { | ||
appendMiloStyles(); |
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.
if you feel, remove appendMiloStyles
, and maybe reference the styles in question in the test page.
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.
finally someone will do that later, as it's more work than i was expected :) and not directly related to my work
|
||
export class Catalog extends VariantLayout { | ||
constructor(card) { | ||
super(card); |
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.
do we need this constructor?
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.
yes we do! the constructor adds the style if not there
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.
but when you don't declare, it should call the one from VariantLayout no?
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 wanted to shout NO, but i tried and yes it works :) sorry to be such a java guy :)
super(card); | ||
} | ||
|
||
getGlobalCSS() { |
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.
suggestion: getGlobalStyles
?
issues seem fixed. Will mark it verified. |
@afmicka postponed heavy styling changes to later PR (cc @yesil ) so no additional testing on your hand is needed :) |
* MWPW-156612 fragment merch-card variant layouts - move from merch-card.js code each layout code related to a given variant under variants/<variant> class - move from global.css.js css related to a given variant under variants/<variant>.css.js - move from merch-card.css.js wc css related to a given variant under variants/<variant>.variantStyle - uses variants/variants as an index to get related class from related variant, and wc style too, - uses variants/VariantLayout, superclass of all variants, to insert css if card is used, and hold common code * MWPW-156612 changing title selector to something independent of the layout (not initialized when used at first in collections) * MWPW-156612 forking style insertion * MWPW-156612 styles fixes * MWPW-156612 review comments * MWPW-156612 fixing unit tests * MWPW-156612 address review comments * MWPW-156612 catalog & image test cov * MWPW-156612 100% coverage for variants * MWPW-156612 fixing (again) ut * MWPW-156612 fix one inline heading style * MWPW-156612 bg img height
variant
classvariant
.css.jsvariant
.variantStyleResolves: https://jira.corp.adobe.com/browse/MWPW-156612
Test URLs:
Before: https://main--milo--adobecom.hlx.page/?martech=off
After: https://MWPW-156612--milo--npeltier.hlx.page/?martech=off
Before: https://main--milo--adobecom.hlx.live/docs/library/kitchen-sink/merch-card?martech=off&georouting=off
After: https://MWPW-156612--milo--npeltier.hlx.live/docs/library/kitchen-sink/merch-card?martech=off&georouting=off
tested kitchen sink with consolidated style in one tag done before first card is even parsed https://test-consolidate--milo--npeltier.hlx.live/docs/library/kitchen-sink/merch-card?martech=off&georouting=off but does not look like it greatly improves TBT