feat: add flex layout support to renderV2Message (DTCRCMERC-5374) - #1367
Conversation
jeremy-herman
left a comment
There was a problem hiding this comment.
Some minor comments from the code review. Will do some testing next!
There was a problem hiding this comment.
These tests were pretty flaky so this is my attempt to fix that. Was noticing that the iframe existed before document.body was ready, this should help smooth that out in CI
Braluna-pp
left a comment
There was a problem hiding this comment.
Approved intent-aware PR review findings: F1, F2, F3, F4.
| if (assets) { | ||
| return assets.map(({ src, dimensions: [width, height] }, idx) => ( | ||
| // eslint-disable-next-line react/no-array-index-key | ||
| <span key={idx} className="pp-flex__logo"> |
There was a problem hiding this comment.
Finding Description
[P1] Size single-piece brand logos as full lockups
The renderer gives every resolved asset the same pp-flex__logo class, but the ratio CSS treats the first child as a small PayPal monogram and the second as the wider wordmark. PayPal Credit and Venmo each resolve to one combined lockup, so that full logo lands in the monogram slot. In 1x1 it is constrained to 29px wide, and in 1x4 to 27px. The PayPal Credit lockup is therefore only about 4px tall.
Problematic Code Snippet(s)
return assets.map(({ src, dimensions: [width, height] }, idx) => (
<span key={idx} className="pp-flex__logo">
<img src={src} alt="" role="presentation" width={width} height={height} />
</span>
));Suggested Change(s)
- Implementation: Add brand or asset-shape classes and size single-piece PayPal Credit and Venmo lockups as whole logos instead of by PayPal asset position.
- Test: Cover PayPal Credit, Venmo, and fallback images in portrait and landscape flex ratios, asserting the selected class and rendered dimensions.
Relevant Links
| return linkClassName ? <span className={linkClassName}>{item.text}</span> : <span>{item.text}</span>; | ||
| case 'TEXT': | ||
| return item.brand ? <strong>{item.text}</strong> : item.text; | ||
| default: |
There was a problem hiding this comment.
Finding Description
[P1] Render supported TEXT_VARIABLE blocks
The renderer handles IMAGE, LINK, and TEXT, then returns item.text for every other type. The repository's supported TEXT_VARIABLE fixture contains name: 'installment_amount' and no text, so this branch returns undefined. Flex sends every non-image block through this helper, causing valid variable content to disappear from both the visible message and its accessible label.
Problematic Code Snippet(s)
case 'TEXT':
return item.brand ? <strong>{item.text}</strong> : item.text;
default:
return item.text;Suggested Change(s)
- Implementation: Implement the CPS
TEXT_VARIABLEcontract at this boundary, passing any required resolution context instead of silently returning an absenttextfield. - Test: Render a resolved variable between two text blocks and assert its visible position and inclusion in the accessible label. Add a defined unresolved-variable failure case.
Relevant Links
There was a problem hiding this comment.
Need to push back on this one a bit - CPS/UCG already merges TEXT_VARIABLE → TEXT unless the client opts into TEXT_VARIABLES_*. CPNW does not request those features, and adaptCpsPresentmentResponse rejects TEXT_VARIABLE entirely. The cited renderer switch is PMCs deferred-v6 soft-fallback, not a CPNW contract gap. Can go into more detail if you'd like more clarification on this.
| const actionItems = v2Content?.action_items ?? []; | ||
| const disclaimerItems = v2Content?.disclaimer_items ?? []; | ||
|
|
||
| const logoBlock = mainItems.find(item => item.type === 'IMAGE'); |
There was a problem hiding this comment.
Finding Description
[P1] Preserve ordered image blocks
The flex renderer finds one image, filters every image out of main_items, and renders the selected image before the messaging container. A valid TEXT, IMAGE, TEXT sequence therefore becomes IMAGE, TEXT, TEXT, and any second image is removed completely. This breaks the ordered typed-block contract instead of rendering CPS content directly.
Problematic Code Snippet(s)
const logoBlock = mainItems.find(item => item.type === 'IMAGE');
const mainBlocks = mainItems.filter(item => item.type !== 'IMAGE');Suggested Change(s)
- Implementation: Preserve the ordered block stream. Extract only a contract-defined standalone logo, or render images in place and use layout classes for visual placement.
- Test: Assert DOM order for
TEXT, IMAGE, TEXTand define explicit behavior for multiple images so no valid block is silently discarded.
Relevant Links
There was a problem hiding this comment.
The find/filter behavior is intentional flex presentation, not a contract violation. Flex (like v5 and text left/top/right) extracts the brand logo into a dedicated logo container and renders the remaining blocks as messaging. Order-preserving in-stream IMAGE rendering is the inline path (buildLogoConfiguration + text layout), which has an explicit TEXT, IMAGE, TEXT test. Flex has no logo.position option, and CPNW does not request INLINE_LOGO for flex - so the mid-stream TEXT, IMAGE, TEXT shape the finding assumes is not the flex content contract.
CPS samples for this path use a single logo IMAGE. Dropping additional IMAGE blocks is theoretical vs real flex traffic, not evidence that flex must render an ordered multi-image stream.
| <div className="pp-flex__background" /> | ||
| <div className="pp-flex__content"> | ||
| {logoBlock ? ( | ||
| <div className="pp-flex__logo-container" aria-hidden="true"> |
There was a problem hiding this comment.
Finding Description
[P1] Keep the logo alternative text accessible
mainLabel is built only from mainBlocks after the image has been removed, and the separate logo container is aria-hidden. For the normal leading PayPal image, its alternative_text is therefore absent from all flex accessible output even though the text renderer exposes the same brand label. A message such as PayPal Pay Later is announced only as Pay Later.
Problematic Code Snippet(s)
const mainLabel = buildContentLabel(mainBlocks);
<div className="pp-flex__logo-container" aria-hidden="true">
{renderFlexLogo(logoBlock, color)}
</div>Suggested Change(s)
- Implementation: Expose the logo's
alternative_textonce, either through the main label in source order or through a labeled logo container with decorative child images. - Test: Assert the full accessible name for a standard leading logo and for an unknown image with custom alternative text.
Relevant Links
| className={`logo inline wordmark ${getLogoBrandClass({ | ||
| logoName: item.name, | ||
| alternativeText: item.alternative_text | ||
| })}`.trim()} |
There was a problem hiding this comment.
Nit: could we make the optional brand class explicit instead of relying on .trim() to remove the trailing space? Pulling it into a variable and joining the truthy classes makes this easier to scan:
const brandClass = getLogoBrandClass({
logoName: item.name,
alternativeText: item.alternative_text
});
const className = ['logo', 'inline', 'wordmark', brandClass].filter(Boolean).join(' ');|
|
||
| const banner = iframeBody.querySelector(bannerSelectors.container); | ||
| return ( | ||
| banner?.clientHeight > 0 && { |
There was a problem hiding this comment.
Could we make this readiness check include the things that affect the screenshot instead of relying on the fixed 500 ms sleep below? A positive size proves the body exists, but fonts or images can still finish afterward and change the layout. Waiting inside the iframe for document.fonts.ready, incomplete images, and stable dimensions across two animation frames would remove the remaining timing guess.
There was a problem hiding this comment.
5dfcb44 - will probably need to iterate on this but got rid of the timeout
Braluna-pp
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback, after ci passes you should be good to go
|
🎉 This PR is included in version 1.93.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
This PR is an attempt to cherry pick the flex changes from Laya's PR onto the latest develop.
Original Description
Adds flex layout support to
renderV2Message(DTCRCMERC-5374), following the same static CSS/class-based approach used by the text layout renderer. The flex renderer does not depend ongetMutations,applyCascade, locale/product mutation files, or any v5 mutation cascade logic.Changes:
src/server/v2/message.jsx— addsFlexMessagecomponent that extracts logo frommain_itemsand renderspp-flex__logo-container,pp-flex__messaging,pp-flex__main,pp-flex__action, andpp-flex__disclaimersections directly from CPS v2 content blockssrc/server/v2/styles.js— addsflexStyles()covering all 7 color themes (blue,black,white,white-no-border,gray,monochrome,grayscale) and all 4 ratio layouts (1x1,1x4,8x1,20x1) with responsive media queries; color and ratio are applied via static CSS class selectors, not dynamic injectionsrc/server/v2/getParentStyles.js— existingratioMapalready handles flex ratio outer iframe sizing; no changes neededsrc/server/message/font.js— adds sharedbuildFontRulesutility used by v2 stylesheets (text and flex)src/server/v2/utils/buildLogoConfiguration.js— updates to acceptlogoTypeand correctly handle inline logo extractionColor/ratio values are normalized before reaching
renderV2Messageby the existingvalidateStylelogic (grey→gray,greyscale→grayscale).Author's Note
This fork was fairly out of date so this PR captures the additional effort to address rendering issues that popped due to all of the differences. The existing font and logo logic was broken when I put it on the latest develop and I also tried cleaning up the flex styles CSS & ratio logic. The files changed as a result look completely different.
Screenshots / Videos
I'm using the v5 test harness to render these messages out
Testing instructions
Stage Tag
Test with tag
v5_rewrite_77046c1v5 Harness
https://localhost.paypal.com:8080) ingetMessagingBundleTest page
node scripts/preview-v2-render.jsapparently also can test this but I will be honest - I have no clue how to get this to workReview
The expected SLA for reviews in this repo is days.
All pull requests require an initial review from your team followed by code owner approval.
While initial review is ongoing, please add the following label to your PR
Needs initial review. This initial review process should ensure that:Once you have received initial approval, please do the following:
Needs initial reviewNeeds codeowner reviewCode owners will then review the PR in accordance with our SLA. This process helps maintain code quality and reduces review cycles.