image block with custom variant - #729
Merged
Merged
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-12T11:40:52.907Z |
davids-ensemble
requested changes
Aug 12, 2026
davids-ensemble
left a comment
davids-ensemble
left a comment
Collaborator
There was a problem hiding this comment.
A few changes I would want to see implemented:
- let's call it "Standalone Image" instead of just "Image"
- let's require a caption:
- second row in the table or similar
- hide the caption by default (display: none)
- generate and set an
idon the caption - add
aria-labelledbyreferencing that id value to the image
Main reason for this is to align it with DevDocs' Image Block
In the future, if anyone will require it, we can offer an option to show the caption as well.
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-14T06:24:08.977Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-14T06:50:32.075Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-14T10:39:03.646Z |
davids-ensemble
previously approved these changes
Aug 14, 2026
davids-ensemble
left a comment
davids-ensemble
left a comment
Collaborator
There was a problem hiding this comment.
This looks good to me but I want @melissag-ensemble to also take a quick look.
Collaborator
There was a problem hiding this comment.
We should confirm design sign-off before implementation on this. Let's discuss in slack
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T06:15:56.249Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:12:09.592Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:13:47.647Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:24:17.129Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:27:18.889Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:42:48.876Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:49:30.791Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T07:56:58.774Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-17T08:00:25.884Z |
melissag-ensemble
requested changes
Aug 17, 2026
melissag-ensemble
left a comment
melissag-ensemble
left a comment
Collaborator
There was a problem hiding this comment.
Holding review until scope is confirmed — this PR adds a medium variant and a caption beyond the image variant Madhur approved. Can you settle the design for those with Madhur first, then re-request review? (Context in Slack.)
davids-ensemble
added
the
pending-product-discussion
melissag-ensemble
requested changes
Aug 26, 2026
| display: flex; | ||
| justify-content: center; | ||
|
|
||
| &.medium img { |
Collaborator
There was a problem hiding this comment.
Only .medium gets a width here — the default image variant has no max-width/width on the img, so large images overflow the viewport instead of scaling down (see screenshot).
Collaborator
There was a problem hiding this comment.
width: 80% reduces the overflow, but the image isn't centered — it's shifted left (screenshot).
| img.setAttribute('alt', captionText); | ||
| } | ||
|
|
||
| captionRow.remove(); |
Collaborator
There was a problem hiding this comment.
Caption and alt text aren't interchangeable — alt is an invisible attribute for screen readers, while a caption renders visibly for everyone. This takes the caption row, writes it into the image's alt (overwriting whatever alt the author already set on the image), and then removes the row so the caption never renders. The design doc shows the caption as visible text under the image. Was the intent to render the caption, and leave the image's own alt alone?
Collaborator
Author
There was a problem hiding this comment.
The second row is only needed to provide the alt text for the image; it is not intended to be displayed as a visible caption. David mentioned adding this second row for consistency to show an alt text, so I added it as a source for the alt text and then remove it after extracting the content.
The design doc does not show this text as a visible caption. As shown in the screenshots, the first block is the title block and the second is the banner block. We don't need to display the second row below the image, so it should be removed from the rendered output.
Also, we should keep the image's existing alt text rather than using the caption row to overwrite it.
Collaborator
There was a problem hiding this comment.
Looking back at David's review, the ask was for the caption row to be hidden with display: none, given an id, with aria-labelledby on the image referencing it (to align with DevDocs' Image Block). The current code instead writes the row's text into alt and removes the row, so it doesn't match — no display: none, id, or aria-labelledby. Could we align it with what David described?
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-27T08:02:54.047Z |
Collaborator
Author
|
@melissag-ensemble , Can you please check the PR? |
| function decorateCaption(block) { | ||
| const rows = block.querySelectorAll(':scope > div'); | ||
| const altRow = rows[1]; | ||
| const img = rows[0].querySelector('img'); |
Collaborator
There was a problem hiding this comment.
Whichever approach we land on, the missing-row cases need guarding. Right now an image block with no second row sets alt="undefined" (the literal string), and an empty block throws at rows[0].querySelector since rows[0] is undefined.
Collaborator
Author
There was a problem hiding this comment.
Okay, we’ll add the default alt text for now.
Collaborator
Author
There was a problem hiding this comment.
@melissag-ensemble , I’ve added the default alt text when row[1] is not present.
Collaborator
There was a problem hiding this comment.
@petheanraj-mitrah Rather than a dedicated alt row + default placeholder, could we use the image's own alt text (set via Image options → Alt text in the Doc) like the other blocks? It flows to <img alt> automatically, keeps all blocks consistent, and avoids the default-placeholder case entirely. @davids-ensemble you'd asked for the aria-labelledby caption row — since img.alt already gives the accessible name, any objection to using it here instead?
Collaborator
There was a problem hiding this comment.
I asked for a second row as caption because it's easier to see in docs.
If I recall correctly, the default way of setting an alt text on the image in Google Docs doesn't show that alt text anywhere in the document, you have to select the image and open its details to see it.
Also, if the image is decorative only the alt text should be empty, not undefined, just alt=""
Collaborator
There was a problem hiding this comment.
@petheanraj-mitrah so we're keeping the row per David's original spec (display: none + aria-labelledby, alt="" when decorative) — the current write-to-alt-and-remove would need adjusting.
melissag-ensemble
requested changes
Aug 31, 2026
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-09-02T06:00:57.430Z |
melissag-ensemble
requested changes
Sep 2, 2026
| } | ||
| altRow.remove(); | ||
| } else { | ||
| img.setAttribute('alt', 'Alt text'); |
Collaborator
There was a problem hiding this comment.
Thanks — the crash guard (rows[0]?.) and the undefined case are handled now. The new no-row branch is a problem though: else { img.setAttribute('alt', 'Alt text') } sets a literal alt="Alt text" placeholder, which screen readers read aloud, and it overwrites any alt the author set on the image. My "guard the missing-row case" note just meant don't crash / don't clobber the existing alt — not add a placeholder string. Could we drop the placeholder and leave the image's own alt intact?
| }); | ||
| } | ||
|
|
||
| function decorateCaption(block) { |
Collaborator
There was a problem hiding this comment.
Stepping back on the caption row itself: there's already a standard way to set alt in this project — authors set it directly on the image in Google Docs (Alt text), which flows to <img alt>, and every other block reads that (cards.js, columns.js, logo-showcase.js). Here's info-card doing exactly that, no extra row:
@davids-ensemble you'd asked for the aria-labelledby caption row — since img.alt already provides the accessible name without an extra row, any objection to just using that like the other blocks?
…image caption support
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-09-03T07:43:13.731Z |
melissag-ensemble
approved these changes
Sep 4, 2026
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.
Description
Image block with custom variant(medium and background color)
Jira
https://jira.corp.adobe.com/browse/DEVSITE-2519
Test URL
https://devsite-2519--adp-devsite-stage--adobedocs.aem.page/test/petheanraj/image