Conversation
Detects a video URL alongside the picture in each cell (matching the convention already used in bacom-elastic-carousel), renders those cells as clickable cards with a play-icon overlay, and opens the video in Milo's shared modal block on click. Exports the existing PLAY_SVG icon from scripts.js so it can be reused here instead of introducing a new one.
Milo's default modal init only supports fetching a path as an HTML fragment, which fails for cross-origin video URLs. Switch to calling getModal() directly with a plain <video> element as custom content, reusing Milo's dialog shell without any fetching involved. Also probe each video URL before showing the play icon so we never show a play button for a video that can't actually load.
Root-caused with Playwright against the live branch preview: probing video availability during initial page decoration competes with all the other concurrent requests (fonts, martech, elastic-carousel's own videos on the same domain, global nav), and the browser aborts the probe request under that contention, so every video looked unavailable. Deferring the probe until the window load event fires avoids the contention and lets it succeed reliably.
Replaces the old tiled/duplicated parallax-wall rendering with the actual redesign: the first authored cell in row 1 becomes a single featured video card (heading, description, watch-video link, large thumbnail), and every other cell (rest of row 1 + all of row 2) feeds an arrow-based carousel row, reusing the paging pattern already proven in bacom-elastic-carousel. Each card now also supports its own heading/description text, not just an image. Also simplifies video handling: drops the pre-click availability probe (unreliable under real page load contention) in favor of always showing the play icon and surfacing a graceful "unavailable" message inside the modal if playback actually fails.
Fixes description extraction picking the picture-wrapping paragraph instead of the actual body text. Rebuilds the desktop featured card layout to be self-contained: fixed 1200x599 box, centered, media pane fills the full height via stretch instead of collapsing, text pane vertically centers its content, description now wraps instead of truncating, and the watch-video link renders unconditionally with the exact type spec (14px/700, 3x6px chevron).
Play icon now always renders on the featured image (matching the watch-video link fix), sized up to 64px per the Figma reference instead of the small 36px carousel-card size.
Constrains the carousel to the same 1200px width as the featured card above it and sizes each card so exactly 3 fit per row on desktop, with the rest reachable via the arrow controls.
Removes the authored margin-left offset on the carousel container (a leftover from the old peek-based layout) that was pushing it out of alignment with the featured card above. The carousel now shares the same left edge and centered 1200px width, so the first 3 cards line up exactly under the featured card with the 4th peeking on the right as intended. Also sets the gap between the featured card and the carousel to 8px.
Moves the gray background and border-radius from the text pane to the outer featured container, with 24px padding around everything, instead of a separate rounded gray box on the left with a visible white gap before the image.
Carousel card thumbnails now use 16px radius and #F6F6F6 background matching the featured card, and every card gets the play icon top-right regardless of whether videoSrc resolved.
Uses the precise Figma-exported values (295.749px height, 195/146 aspect ratio) instead of the approximate 1:1 aspect-ratio.
Text block was only appended when heading or description happened to extract as truthy, hiding it entirely for cards where that failed. Now always renders. Watch-video link also shown unconditionally, matching the featured card. Text area gets 24px padding with border-box sizing to stay within the 347x152 box.
The heading was rendering all along but was styled identically to the description (16px/400 gray), making it visually indistinguishable and appear missing. Gives it its own Title 6 spec (24px/900 Adobe Clean Display) and removes truncation so it wraps instead.
Adds 8px gap between heading and description, 3-line clamp on the description (was single-line truncation), and 24px gap before the watch-video link. Switches the text area from a fixed 152px height to min-height so the watch-video link doesn't get pushed out/hidden once the heading wraps and description spans multiple lines.
updateArrowState's initial call ran synchronously while the carousel container was still detached from the document, so scrollWidth and clientWidth both read 0, making the max-scroll check incorrectly conclude there was nothing left to scroll to. Deferring the initial check to requestAnimationFrame so it runs after the element is actually connected and laid out.
Replaces the fixed 1200px width + auto margin (which hard-clipped the row exactly at the featured card's right edge) with just a matching margin-left offset, so the carousel still starts aligned under the featured card but extends to the section's right edge, letting extra cards peek out instead of being cut off artificially.
The card row now bleeds past 1200px to peek the next card, but the arrow controls should stay put at the same right boundary as the featured card above, so giving them an explicit 1200px width.
Image zoom and watch-link chevron shift were scoped to .has-video, which only gets added when videoSrc resolves. Since every card is meant to feel interactive per the Figma behavior notes, these hover effects now apply to any featured/carousel card on hover.
Desktop intentionally lets a 4th card peek past the edge, but tablet should show a clean 3-across row. Sizes cards as a percentage of the container instead of a fixed rem value so they always divide evenly into 3 with no partial card showing at rest.
Mobile was rendering the same featured-card-plus-row-below structure as tablet/desktop, just resized, which doesn't fit a narrow viewport. Now on mobile the featured cell is just the first card in one continuous swipeable carousel alongside every other card, sized to 82vw so neighboring cards peek on both edges, with no arrow controls since it's touch/swipe driven.
The first cell now authors an h3 + paragraph before its picture, representing the overall section header (e.g. "Leadership POV"), separate from the card's own heading/description which live after the picture. Extraction now splits a cell's content into before-picture (section header) and after-picture (card content) groups instead of grabbing the first heading/paragraph found anywhere in the cell. Section header renders centered above the featured card on desktop/tablet, styled per the exact Title 3 spec (56px/900 Adobe Clean Display).
Mobile now uses the s2a heading-2/body-lg tokens, left-aligned, instead of the centered 56px/18px desktop treatment, which is now scoped to the tablet+ breakpoint where it was actually verified.
Desktop/tablet keep the 104px top padding; mobile now gets its own 80px top padding plus 24px left/right.
Text was running edge-to-edge with no breathing room against the card boundaries.
Adds 24px left padding to the carousel container on mobile (reset back to 0 on tablet/desktop, which use their own margin-based alignment) so the row isn't flush against the viewport edge. Section heading and subtext now use Adobe Clean explicitly instead of the s2a font-family tokens, and heading is 32px on mobile.
Mobile swipe carousel was snapping cards to the left edge (scroll-snap-align: start), same as the desktop/tablet arrow-paging carousel. Mobile now snaps to center instead; tablet/desktop keep start alignment since that's what the arrow controls expect.
Covers section header extraction, featured card video-source resolution, carousel card building (play icon + watch link always present), arrow controls showing only when there are more than 3 cards, the mobile single-swipeable-row structure, and the missing-structure error path.
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits
|
|
Brandon32
approved these changes
Aug 3, 2026
JasonHowellSlavin
left a comment
JasonHowellSlavin
left a comment
There was a problem hiding this comment.
Before I continue to review, there is so much net new css and js, we might consider making this it's own block? If it were, could we simplify the authoring pattern? Could we reuse carousel? Just thinking out loud.
At the current stage, it might be easier just to port this code to a new block name and call it a day.
| margin: 0 0 8px; | ||
| color: var(--s2a-color-content-title); | ||
| text-align: left; | ||
| font-family: "Adobe Clean", sans-serif; |
There was a problem hiding this comment.
I find this odd. Is not "Adobe Clean" the default? Or is it adobe black. I might consider keeping all fonts the same. I see the difference in the Figma, but we might want to talk to Jared about it.
|
|
||
| .bento-grid .bento-section-subtext { | ||
| margin: 0; | ||
| color: var(--s2a-color-content-body-strong); |
There was a problem hiding this comment.
i feel as though inheriting color might be best here, since it is just #000
| } | ||
|
|
||
| .bento-grid .foreground .grid-row { | ||
| .bento-grid .foreground .grid-view.view-mobile { |
There was a problem hiding this comment.
might we want to name space the bento block with a specific variant name instead of overwriting all of the core styles that ported over with it?
| height: 2.25rem; | ||
| border-radius: 50%; | ||
| background: rgb(0 0 0 / 40%); | ||
| color: #FFF; |
There was a problem hiding this comment.
we may want to add to our CLAUDE.md or elsewhere a preference for either lowercase or uppercase hex codes. I feel like the linter does prefer one.
Comment on lines
+186
to
+189
| top: 1.5rem; | ||
| right: 1.5rem; | ||
| width: 64px; | ||
| height: 64px; |
There was a problem hiding this comment.
not a huge fan of mixing values like px, rem, em. Feels like most of Milo preferes px, but I know c2 is using rem
Comment on lines
+273
to
+274
| -webkit-line-clamp: 3; | ||
| -webkit-box-orient: vertical; |
There was a problem hiding this comment.
anyway to achieve this without the vendor prefix?
| gap: 1rem; | ||
| .bento-grid .bento-featured { | ||
| width: 1200px; | ||
| height: 599px; |
There was a problem hiding this comment.
599px feels very much like a magic number
| } | ||
|
|
||
| .bento-grid .grid-item-media { | ||
| height: 295.749px; |
There was a problem hiding this comment.
Let's please round this to something that makes sense.
| .bento-grid .grid-item-media { | ||
| height: 295.749px; | ||
| align-self: stretch; | ||
| aspect-ratio: 195 / 146; |
There was a problem hiding this comment.
this is not likely to work for every asset provided by creative. Lets remove this or find another way to fix the media to the desired width or height
Comment on lines
+296
to
+302
| if (isRtl()) { | ||
| prevBtn.disabled = scrollLeft >= -1; | ||
| nextBtn.disabled = scrollLeft <= -maxScroll + 1; | ||
| } else { | ||
| prevBtn.disabled = scrollLeft <= 1; | ||
| nextBtn.disabled = scrollLeft >= maxScroll - 1; | ||
| } |
There was a problem hiding this comment.
early return to flatten:
if (isRtl()) {
prevBtn.disabled = scrollLeft >= -1;
nextBtn.disabled = scrollLeft <= -maxScroll + 1;
return;
}
prevBtn.disabled = scrollLeft <= 1;
nextBtn.disabled = scrollLeft >= maxScroll - 1;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: MWPW-202490
Test URLs
Before: https://main--da-bacom--adobecom.aem.live/drafts/jsandlan/elastic?martech=off
After: https://bento-box--da-bacom--adobecom.aem.live/drafts/jsandlan/elastic?martech=off