Skip to content

feat(DTCRCMERC-4687): implement v5 text-layout styles for renderV2Message#1331

Open
sailaya99 wants to merge 2 commits into
paypal:developfrom
sailaya99:DTCRCMERC-4687-v5-styles-render-v2-message
Open

feat(DTCRCMERC-4687): implement v5 text-layout styles for renderV2Message#1331
sailaya99 wants to merge 2 commits into
paypal:developfrom
sailaya99:DTCRCMERC-4687-v5-styles-render-v2-message

Conversation

@sailaya99

Copy link
Copy Markdown
Collaborator

Description

Implements v5 text-layout styles for the renderV2Message SSR module (DTCRCMERC-4687).

Previously, renderV2Message rendered with a bare .pp-message container and minimal CSS — no color treatment, no logo positioning styles, no font configuration, and no alignment support. This PR introduces a static, class-based stylesheet (no legacy mutation system) that covers all v5 text style options:

  • text.color → CSS class on .main/.action spans (black, white, monochrome, grayscale)
  • text.size → font-size in px (10–16)
  • text.align → text-align on .pp-message (left, center, right)
  • logo.position → margin/display treatment on .logo (left, right, top)
  • logo.type → passed through as a root data-pp-style-logo-type attribute
  • text.fontFamily / text.fontSource → @font-face generation with https-only URL validation and XSS-safe font name filtering
  • greyscale alias normalized to grayscale via validateStyle before reaching render()

Root data-pp-style-* attributes are added to the .pp-message div for downstream observability and potential CSS hook use.

…sage

- Add body reset and scoped .pp-message stylesheet (display:block, width:100%)
- Map text.color to CSS classes on .main/.action spans with vertical-align:middle
- Add CSS filters on .logo img for white (invert), monochrome (grayscale+black), grayscale variants
- Handle logo positions: left (default), right (margin swap), top (block display)
- Implement fontSource @font-face generation with URL/name security validation
- Wire fontSource through message.jsx into styles()
- Add data-pp-style-* root attributes for layout, logo-type, logo-position, text-align, text-color, text-size
- Add greyscale alias pipeline test (validateStyle -> render -> class)
- Expand snapshot coverage to all 7 text sizes (10-16) and full logo/color/align matrix (21 snapshots)

@Braluna-pp Braluna-pp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thermo-nuclear code quality review: a few maintainability and coverage issues worth tightening before this lands.

text: { color: 'black', size: 12, align: 'left' }
}
};
expect(render(options, contentWithLogo)).toMatchSnapshot();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This snapshots the full rendered HTML for every matrix case, which creates a 1,240-line snapshot file because each case repeats the entire inline stylesheet. That makes future CSS changes noisy: one real stylesheet change will rewrite many snapshot blocks.

Problematic pattern:

expect(render(options, contentWithLogo)).toMatchSnapshot();

Can we keep one full representative render snapshot, snapshot the stylesheet once, and use focused matrix assertions for the cases? For example:

test('renders the v2 stylesheet once', () => {
    const result = render(baseOptions, baseV2Content);
    expect(result.match(/<style>[\s\S]*<\/style>/)[0]).toMatchSnapshot();
});

test.each([['primary'], ['alternative'], ['inline'], ['none']])('maps logo type: %s', logoType => {
    const result = render(
        {
            style: {
                layout: 'text',
                logo: { type: logoType, position: 'left' },
                text: { color: 'black', size: 12, align: 'left' }
            }
        },
        contentWithLogo
    );

    expect(result).toContain(`data-pp-style-logo-type="${logoType}"`);
});

That keeps the acceptance coverage while avoiding a huge low-signal snapshot file.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done — snapshot file is down to 119 lines. Kept one full render snapshot, one stylesheet snapshot, and replaced the matrix cases with test.each + toContain() assertions so a CSS change only touches the two snapshot blocks, not every case.

Comment thread src/server/v2/message.jsx
<div
className="pp-message"
data-pp-style-layout={style.layout}
data-pp-style-logo-position={logoPosition}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This emits data-pp-style-logo-position="inline" when logo.type is inline, because inline is being modeled as a pseudo-position. That muddies the contract: inline is a logo type, while the accepted v5 positions are left, right, and top.

Problematic source:

const logoPosition = style.logo?.type === 'inline' ? 'inline' : style.logo?.position ?? 'left';
const logoType = style.logo?.type ?? 'primary';

Can we keep type and position separate and pass both into the logo helper? For example:

const logoType = style.logo?.type ?? 'primary';
const logoPosition = style.logo?.position ?? 'left';

const { logoBlock, hasInitialLogo, hasRightLogo, mainBlocks } = buildLogoConfiguration({
    logoType,
    logoPosition,
    mainItems
});

Then the helper can branch on logoType === 'inline' without inventing a fourth position:

export function buildLogoConfiguration({ logoType, logoPosition, mainItems }) {
    if (logoType === 'inline') {
        return { hasInitialLogo: false, hasRightLogo: false, logoBlock: undefined, mainBlocks: mainItems };
    }

    const logoBlock = mainItems.find(item => item.type === 'IMAGE');
    const mainBlocks = mainItems.filter(item => item.type !== 'IMAGE');

    return {
        hasInitialLogo: !!logoBlock && (logoPosition === 'left' || logoPosition === 'top'),
        hasRightLogo: !!logoBlock && logoPosition === 'right',
        logoBlock,
        mainBlocks
    };
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. message.jsx now derives logoType and logoPosition independently and passes both to buildLogoConfiguration. The helper branches on logoType === 'inline' directly without inventing a fourth position value, so data-pp-style-logo-position always reflects the real position.

}
};
const result = render(options, baseV2Content, mockLog);
expect(result).toContain('data-pp-style-layout="text"');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This proves the SSR output contains the expected data attributes, but it still does not prove AC 12: merchant page CSS cannot override the v2 text message stylesheet under the normal SDK iframe/message rendering path.

Current coverage is essentially:

expect(result).toContain('data-pp-style-layout="text"');
expect(result).toContain('data-pp-style-logo-position="top"');
expect(result).toContain('data-pp-style-logo-type="primary"');

Can we add a small integration-style test around the normal message iframe path, or link this story to an existing test that already proves iframe isolation? Shape of the missing proof:

test('renders v2 message styles inside the iframe document, isolated from merchant page CSS', async () => {
    // Arrange merchant page CSS that would override .pp-message if rendering happened in-page.
    // Render through the normal message iframe path.
    // Assert the iframe document contains the v2 stylesheet and computed styles match v2 CSS.
});

Without that, the PR proves the stylesheet is emitted, but not that merchant CSS cannot override it in the production rendering path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added a v2 render stylesheet isolation describe block with two tests: one asserting the stylesheet is embedded inline (no ), and one parsing the CSS output to assert every selector is scoped to .pp-message or body. This is the extent of what's provable at the unit test level. A true computed-style assertion against merchant CSS overrides would require Playwright/Puppeteer infrastructure not currently in this repo — happy to add it or link to an existing E2E test if one covers this path.

- Separate logoType/logoPosition in message.jsx; buildLogoConfiguration
  now branches on logoType=inline without using it as a pseudo-position
- Replace full-matrix snapshots with one representative render snapshot,
  one stylesheet snapshot, and test.each matrix assertions (119 lines vs ~1240)
- Add stylesheet isolation tests (inline embed + selector scope check)
- Simplify redundant selector-parsing filter in isolation test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants