Skip to content

[Test]: Integration of new PDP component built on Zazzle SDK#304

Open
maxn-adobe wants to merge 11 commits intostagefrom
MWPW-184613
Open

[Test]: Integration of new PDP component built on Zazzle SDK#304
maxn-adobe wants to merge 11 commits intostagefrom
MWPW-184613

Conversation

@maxn-adobe
Copy link
Copy Markdown
Contributor

@maxn-adobe maxn-adobe commented Apr 6, 2026

Summary

This PR introduces the new print-product-detail-sdk block into the collection of blocks. Currently, this block is kept separate from the existing 'print-product-detail' block, for testing purposes. Once design and code review as well as testing are complete, we can rename this block and remove the old one in order to replace it.


Jira Ticket

Resolves: MWPW-184613


Test URLs

Hoodie:
Before:
https://www.adobe.com/express/create/print/hoodie/hot-yellow-and-black-student-council-hoodie
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-hoodie-1-sdk?martech=off

Pillow:
Before:
https://www.adobe.com/express/create/print/pillow/blue-evergreen-pillow
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-pillow-1-sdk?martech=off

Mug:
Before:
https://www.adobe.com/express/create/print/mug/brown-and-white-coffee-mug
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-mug-1-sdk?martech=off

Tote Bag:
Before:
https://www.adobe.com/express/create/print/tote-bag/blue-coding-tote-bag
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-tote-bag-1-sdk?martech=off

Poster:
Before:
https://www.adobe.com/express/create/print/poster/blue-blank-travel-gallery-poster
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-poster-1-sdk?martech=off

Sticker:
Before:
https://www.adobe.com/express/create/print/sticker/green-gift-and-packaging-square-sticker
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-sticker-1-sdk?martech=off

Card:
Before:
https://www.adobe.com/express/create/print/card/brown-polaroid-anniversary-card
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-card-1-sdk?martech=off

Business Card:
Before:
https://www.adobe.com/express/create/print/business-card/blue-and-gold-professional-realtor-business-card
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-business-card-1-sdk?martech=off

Tshirt:
Before:
https://www.adobe.com/express/create/print/t-shirt/green-and-beige-muted-vintage-car-t-shirt
After:
https://mwpw-184613--da-express-milo--adobecom.aem.page/drafts/maxn/pdp-test/pdp-test-page-t-shirt-1-sdk?martech=off


Verification Steps

  • Navigate to both the Before and After link and compare layout and functionality.
  • Note: There is a list of known issues, caused by the differences between the payloads provided by Zazzle's raw API and the payload that is returned from their new SDK. Please find those issues listed in this spreadsheet:

https://adobe-my.sharepoint.com/:x:/p/maxn/IQAKof5yN8A3RLmQyGr-IEj4AemzG-JgImdEZa6IT9kKaFA?e=TwYC3B


Potential Regressions

Once we replace the block, this update will affect all print-product-detail pages.


Additional Notes

@aem-code-sync
Copy link
Copy Markdown

aem-code-sync Bot commented Apr 6, 2026

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@aem-code-sync
Copy link
Copy Markdown

aem-code-sync Bot commented Apr 6, 2026

@maxn-adobe maxn-adobe marked this pull request as ready for review April 6, 2026 23:21
@maxn-adobe maxn-adobe added the Ready for Review Ready for peer review. label Apr 6, 2026
Copy link
Copy Markdown
Contributor

@fullcolorcoder fullcolorcoder left a comment

Choose a reason for hiding this comment

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

A small pile of changes requested here.

Also:

The PSI bot shows mobile LCP is red (5.6s–7.0s) across every test page, and desktop CLS hits 0.369 (red) on t-shirt. Before this replaces the live block, it's worth running a comparison PSI run against the "Before" URLs to understand the delta — and diagnosing what's driving the LCP regression. The Preact render happening before CSS loads (widgets are loaded asynchronously after render()) and the hero image not getting fetchpriority="high" treatment from the initial HTML are likely contributors. Worth investigating before production promotion.

${state?.descriptionComponents?.[1]?.descriptionHTML && html`
<div
class="pdpx-drawer-description"
dangerouslySetInnerHTML=${{ __html: state.descriptionComponents[1].descriptionHTML }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocking: dangerouslySetInnerHTML is used with descriptionHTML from the Zazzle SDK API in two places (ProductComponents.js and CustomizationInputs.js). That field is documented as API-supplied HTML. Without sanitization, this is a direct XSS vector if the API response is ever compromised or unexpected. Your own useSeo.js already has a safe stripHtml using DOMParser — the same pattern should be applied here, or use DOMPurify if a richer subset of tags needs to be preserved.

Both here and in CustomizationInputs.js you have:

// ProductComponents.js ~1658
dangerouslySetInnerHTML=${{ __html: state.descriptionComponents[1].descriptionHTML }}

// CustomizationInputs.js ~848
dangerouslySetInnerHTML=${{
  __html: selector.preview.descriptionHTML,
}}

Needs some refactoring with

// Add a sanitizer — DOMPurify is the standard choice.
// If DOMPurify isn't available, use the existing stripHtml pattern from useSeo.js
// which uses DOMParser to strip all tags safely.

// Option A: DOMPurify (if vendored or available)
import DOMPurify from 'dompurify';
dangerouslySetInnerHTML=${{ __html: DOMPurify.sanitize(state.descriptionComponents[1].descriptionHTML) }}

// Option B: use the stripHtml pattern already in useSeo.js
// function stripHtml(html) {
//   const doc = new DOMParser().parseFromString(html, 'text/html');
//   return doc.body.textContent || '';
// }
// If only <li>, <p>, <strong> etc. are expected, you could allow a controlled subset.
// But if only text content is needed, stripHtml is safest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a sanitizeHtml utility using DOMParser that allowlists safe tags (p, br, strong, em, ul, ol, li, etc.) and applied it at both dangerouslySetInnerHTML sites.

if (!templateId) {
return;
}
fetchProduct(templateId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocking: fetchProduct(templateId) in PDPContent's useEffect has no .catch(). If the Zazzle SDK fails to fetch the product (network error, bad ID, SDK issue), the page will hang on the loading skeleton forever with no user feedback or logging. Add a .catch() that calls lana.log and sets an error state so users see something actionable.

Something like:

useEffect(() => {
  fetchProduct(templateId).catch((err) => {
    window.lana?.log(`print-product-detail-sdk: fetchProduct failed: ${err.message}`, { level: 'error' });
    // Set an error state so the UI can show a user-facing error instead of hanging
    setFetchError(true);
  });
}, [templateId]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a .catch() that logs via lana and sets an error state so the UI renders a user-facing error message instead of hanging on the skeleton.

onRequestDrawer=${onRequestDrawer}
productType=${productType}
/>`;
case 'radio':
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

blocking: renderAttribute sends both 'radio' and 'checkbox' cases to DropdownSelector, but RadioSelector and CheckboxSelector are already implemented in this file and never called. Looks like the switch cases were written but the component references weren't updated from the template. If the SDK ever returns those selector types, users would get a dropdown instead of the correct control.

case 'radio':
  return html`<${RadioSelector} attribute=${attribute} />`;
case 'checkbox':
  return html`<${CheckboxSelector} attribute=${attribute} />`;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was done intentionally. We currently dont have Designs for radio or checkbox inputs, in the PDP experience. These types of inputs were never imagined / designed for previously. The input type values are passed to us from Zazzle so we need to either design treatments for those input types, or simply override them with input types that we have designed for, which is what was done here.


useEffect(() => {
let active = true;
import('../../../scripts/utils/location-utils.js')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The country detection promise chain in CheckoutButton has no .catch(). If the location-utils import or getCountry() fails, outOfRegion stays null and the checkout button renders nothing — no CTA, no error message. Consider defaulting to setOutOfRegion(false) on failure so at least the button shows, even if region-gating is temporarily unavailable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a .catch() that logs via lana and defaults outOfRegion to false so the checkout button still renders if the geo lookup fails.

}, [
attribute.name,
title,
selector.optionGroups?.map((g) => (g.options || []).map((o) => o.value).join(',')).join('|'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The imperative pill-building useEffect in MiniPillCarousel (and PaperTypeContent) attaches click handlers that close over selectedOptionValue, but selectedOptionValue isn't in that effect's dependency array. A separate useEffect correctly updates the visual selected state, but the tracking calls and the value !== selectedOptionValue guard in the click handler run on a stale captured value. The ref pattern (useRef for always-current values in imperative event handlers) is the clean fix here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a handlersRef/actionsRef pattern so imperatively-attached event handlers always delegate to the latest closures rather than stale captured values.

Comment on lines +869 to +907
.size-chart-product-name {
font-size: var(--body-font-size-l);
font-weight: var(--subheading-font-weight);
color: var(--color-gray-950);
margin: 0;
margin-bottom: var(--spacing-100);
}
.size-chart-table-container {
border-radius: 16px;
border: 1px solid var(--color-gray-325);
padding: var(--spacing-400);
color: var(--color-gray-900);
margin-bottom: var(--spacing-400);
}
.size-chart-table-section {
display: flex;
flex-direction: column;
gap: var(--spacing-100);
}
.size-chart-table thead .size-chart-table-header {
font-size: var(--body-font-size-s);
font-weight: var(--subheading-font-weight);
color: var(--color-gray-950);
text-align: left;
padding: var(--spacing-100);
white-space: nowrap;
}
.size-chart-table {
width: 100%;
border-collapse: separate;
border-spacing: 0 var(--spacing-100);
table-layout: fixed;
}
.size-chart-table thead th {
font-size: var(--body-font-size-s);
font-weight: normal;
color: var(--color-gray-800);
text-align: left;
padding: var(--spacing-100);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Scope to block (and the ones below this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Scoped .hidden, [data-skeleton='true'], and all .size-chart-* selectors under .print-product-detail-sdk.

<span class="pdpx-ratings-number" id="pdpx-ratings-number" aria-hidden="true">${formattedRating}</span>
</div>
<div class="pdpx-ratings-amount-container">
<button class="pdpx-ratings-amount" id="pdpx-ratings-amount" type="button" aria-label="${formattedCount} reviews">${formattedCount}</button>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reviews count renders as a with no onClick and cursor: default in CSS. Screen reader users will tab to it and hear "123 reviews, button" with no action available. If it's meant to scroll to a reviews section eventually, add the handler — if not, swap it for a .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the reviews count from a button to a span since there's no click handler.

class="pdpx-drawer ${state.open ? '' : 'hidden'}"
id="pdp-x-drawer"
role="dialog"
aria-modal="${state.open ? 'true' : 'false'}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

aria-modal="false" has no defined semantics in the ARIA spec and is handled inconsistently across screen readers. Since the .hidden class sets display: none, the element is already removed from the accessibility tree when closed. Just set aria-modal="true" statically.

Copy link
Copy Markdown
Contributor Author

@maxn-adobe maxn-adobe Apr 14, 2026

Choose a reason for hiding this comment

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

Changed to a static aria-modal="true" since the .hidden class already removes the element from the accessibility tree when closed.

@media (hover: none), (pointer: coarse) {
.pdpx-mini-pill-image-container::after,
.pdpx-mini-pill-image-container::before {
display: none !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This signals a specificity collision. Move the tooltip ::after/::before rules inside a @media (hover: hover) condition instead of suppressing them with !important in the touch query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the @media (hover: none) override and instead wrapped the tooltip ::after/::before rules inside @media (hover: hover) so they never apply on touch devices in the first place.

document.addEventListener('keydown', handleKeyDown);

// Move focus into the drawer
requestAnimationFrame(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the drawer content (e.g. SizeChart) is still loading when the frame fires, focus may move to an element that isn't interactive yet. There's also no active guard inside the rAF callback, so a rapid open/close sequence before the frame fires moves focus to the (now hidden) drawer. A setTimeout(0) with an active flag, or checking drawer.contains(document.activeElement) after focus, would be more robust.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced requestAnimationFrame with setTimeout(0) and added an active flag that's set to false in cleanup, so rapid open/close won't focus a hidden drawer.

@echen-adobe
Copy link
Copy Markdown
Contributor

echen-adobe commented Apr 21, 2026

  1. There seems to be some stutter when opening the rows.
Screen.Recording.2026-04-21.at.2.58.53.PM.mov
  1. The options carousel here seems to have some focus state issues, it is also no longer responding to the enter key.
Screenshot 2026-04-21 at 3 01 26 PM
  1. There looks to be some flashing caused by changing the pills at the bottom
Screen.Recording.2026-04-21.at.3.04.48.PM.mov
  1. Some more weird focus states.
Screenshot 2026-04-21 at 3 04 14 PM

Copy link
Copy Markdown
Contributor

@echen-adobe echen-adobe left a comment

Choose a reason for hiding this comment

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

Noticed several visual issues.

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.

3 participants