feat(DTCRCMERC-5374): add flex layout support to renderV2Message#1339
feat(DTCRCMERC-5374): add flex layout support to renderV2Message#1339sailaya99 wants to merge 5 commits into
Conversation
…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)
- 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
Implements FlexMessage component and flex stylesheet in src/server/v2, using a static CSS/class-based approach with no dependency on the v5 mutation cascade (getMutations, applyCascade, or locale mutation files). - FlexMessage maps color and ratio to CSS classes and data attributes - flexStyles() covers all 7 color themes and 4 ratio layouts with responsive media queries - getParentStyles ratioMap drives outer iframe/container aspect-ratio sizing for flex ratios - grey/greyscale aliases normalize via existing validateStyle logic - Unit tests cover all color/ratio combinations, structure, stylesheet isolation, alias normalization, and snapshots for representative cases
Braluna-pp
left a comment
There was a problem hiding this comment.
Thermo-nuclear code quality review focused on maintainability, structure, and reviewability. These are not behavior blockers from a test run; they are structural review comments from the PR diff and Jira scope.
| }); | ||
| } | ||
|
|
||
| function flexStyles({ fontFamily, fontSource } = {}) { |
There was a problem hiding this comment.
This adds a large hand-written flex stylesheet directly into the shared v2 styles module. The file now mixes font setup, text styles, seven color themes, four ratio layouts, and responsive behavior in one template string, which makes future flex changes harder to review because a small theme or ratio change means scanning the whole CSS blob.
Problematic shape:
function flexStyles({ fontFamily, fontSource } = {}) {
// themes, ratios, media queries, and font setup all live here
}Can we split flex into its own module and make the themes/ratios table-driven? For example, keep textStyles and flexStyles in separate files, define FLEX_THEMES and FLEX_RATIOS, then render the selectors from those maps. Same output, but fewer repeated selectors and a clearer owner for flex styling.
There was a problem hiding this comment.
Done. flexStyles.js now has FLEX_THEMES, PORTRAIT_RATIOS, and LANDSCAPE_RATIOS data tables. A lscpSel() helper expands LANDSCAPE_RATIOS into grouped selectors everywhere, including inside @media blocks. CSS output is unchanged; all 95 tests pass.
| const isSafeFontName = value => typeof value === 'string' && /^[\w\s-]+$/.test(value.trim()); | ||
| const isSafeFontSource = value => typeof value === 'string' && /^https:\/\/[^'")\s]+$/i.test(value); | ||
|
|
||
| export function buildFontRules({ fontSource, fontFamily, fallbackStack, defaultFontFamily, fontNamePrefix }) { |
There was a problem hiding this comment.
This puts a second font-generation path into the legacy message font module, but the existing getFontRules path above still uses different naming, fallback, and validation behavior. That leaves one file owning two contracts that can drift.
Problematic shape:
export function buildFontRules(...) { ... }Can we either move this v2-only helper under src/server/v2 or fully refactor getFontRules to use the same shared primitive? Right now the abstraction looks shared, but only v2 uses it, so it adds cross-layer coupling without deleting the old complexity.
There was a problem hiding this comment.
buildFontRules is in src/server/v2/font.js. The only change to src/server/message/font.js on this branch is a trailing-newline removal — no new exports were added there.
| ['gray', '1x4'], | ||
| ['monochrome', '20x1'] | ||
| ])('full render snapshot for flex color=%s ratio=%s', (color, ratio) => { | ||
| const options = { style: { layout: 'flex', color, ratio } }; |
There was a problem hiding this comment.
These full render snapshots add a new 1,235-line snapshot file. That is too much generated output for this change, and it makes the review depend on scanning huge snapshots instead of clear expectations. It also duplicates many string assertions already covered above.
Problematic shape:
expect(render(options, flexContentWithLogo)).toMatchSnapshot();Can we shrink this to one representative HTML snapshot plus semantic assertions for the full color/ratio matrix? The ratio/theme coverage can stay table-driven, but the tests should assert the meaningful contract directly: root classes, data attributes, key selectors, and logo/action/disclaimer structure.
There was a problem hiding this comment.
The snapshot file is 491 lines. The color/ratio matrix uses semantic toContain assertions; only two flex snapshots exist: one full HTML render (blue/8x1) and one for the stylesheet.
| ); | ||
| } | ||
|
|
||
| function FlexMessage({ style, v2Content }) { |
There was a problem hiding this comment.
This adds the whole flex renderer inside message.jsx, while the same file still owns the text renderer and shared block rendering. The branch at style.layout === 'flex' is clean today, but this file is now the place where every future layout can grow more renderer-specific logic.
Problematic shape:
function FlexMessage({ style, v2Content }) { ... }Can we move FlexMessage into a dedicated v2 flex module and keep message.jsx as a small dispatcher? The shared helpers like renderBlock, renderLogo, and content extraction can stay reusable, but the layout-specific structure should live behind its own component so this does not become a multi-layout grab bag.
There was a problem hiding this comment.
FlexMessage is in src/server/v2/flex.jsx. message.jsx just imports and dispatches to it.
| const color = style.color ?? 'black'; | ||
| const ratio = style.ratio ?? '8x1'; |
There was a problem hiding this comment.
This creates a correctness edge case: the direct flex renderer defaults do not match the validated flex defaults. validOptions defaults flex to color: "blue" and ratio: "1x1" because those are the first valid values, but this lower-level render path falls back to black and 8x1.
That means render({ style: { layout: "flex" } }) can produce class="pp-message pp-flex black r-8x1", while the validated path produces blue r-1x1. Since render, validateStyle, and getParentStyles are all exported from src/server/v2/index.js, callers can hit this mismatch and end up with a rendered message that does not match the wrapper/default style contract.
Can we make these defaults come from the same source? At minimum this should use blue and 1x1 here, and ideally the flex defaults should live in one shared constant used by both validOptions and FlexMessage. Please also add a small test for render({ style: { layout: "flex" } }) so this stays aligned.
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).Screenshots / Videos
N/A
Testing instructions
npm test— 152 unit tests pass covering all color and ratio combinations, content block structure, stylesheet isolation, alias normalization, and snapshotsnode scripts/preview-v2-render.jswithstyle.layout: "flex"options