feat(ui): unify content thumbnails into dot-content-thumbnail component (#36295)#36424
feat(ui): unify content thumbnails into dot-content-thumbnail component (#36295)#36424oidacra wants to merge 5 commits into
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
🤖 dotBot Review (Bedrock)Reviewed 37 file(s); 12 candidate(s) → 8 confirmed, 2 uncertain (unverified, kept for review).
Confirmed findings
🔎 Uncertain (could not confirm or disprove — review manually)
us.deepseek.r1-v1:0 · Run: #28683006674 · tokens: in: 161415 · out: 49151 · total: 210566 · calls: 67 · est. ~$0.483 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| hasTitleImage: true | ||
| } as unknown as DotCMSContentlet; | ||
|
|
||
| it('resolves the thumbnail model from a contentlet', () => { |
There was a problem hiding this comment.
🔴 [Critical] Incorrect URL path construction for title images
The test expects that when hasTitleImage is true, the generated URL does not include the 'titleImage' segment. However, the component's logic in dot-content-thumbnail.component.ts shows that hasTitleImage: true should prepend 'titleImage' to the path. The test's expectation is inverted, indicating either a test error or a component bug. Since the test passes with this incorrect expectation, the component may be generating wrong URLs for title images.
| constructor() { | ||
| effect(() => this.stateChange.emit(this.$state())); | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 [Critical] Infinite reload loop on media error
The onMediaError handler sets the state to 'error', but subsequent ngOnChanges triggers loadMedia() again when inputs remain unchanged. This creates an infinite error/reload cycle as failed media attempts repeatedly re-trigger loading without input change validation.
| <div class="h-14 w-20 shrink-0 overflow-hidden rounded-md bg-gray-100"> | ||
| <dot-contentlet-thumbnail | ||
| [iconSize]="'24px'" | ||
| <dot-content-thumbnail |
There was a problem hiding this comment.
🟠 [High] Missing required options in dot-content-thumbnail
The original dot-contentlet-thumbnail had iconSize="24px" and [playableVideo]="true". The replacement dot-content-thumbnail requires these settings via [options] input based on component API documentation in the PR description. Their absence may break icon sizing and video playback behavior in relationship field thumbnails.
| @@ -26,7 +26,8 @@ import { SuggestionsService } from './services'; | |||
| ReactiveFormsModule, | |||
| ButtonModule, | |||
| TagModule, | |||
There was a problem hiding this comment.
🟠 [High] Incorrect Angular module configuration for DotContentThumbnailComponent
DotContentThumbnailComponent is imported directly in SharedModule's imports array. Angular modules should only import other modules, not individual components unless they are standalone. Since DotContentThumbnailComponent is not marked as standalone (based on its declaration in DotContentThumbnailModule), this configuration will cause Angular template errors. The correct approach is to import DotContentThumbnailModule which exports the component.
| it('should render the unified thumbnail with the temp file pdf src', () => { | ||
| spectator.detectChanges(); | ||
|
|
||
| expect(spectator.query(byTestId('contentlet-thumbnail'))).toBeTruthy(); |
There was a problem hiding this comment.
🟠 [High] Test uses legacy test ID after component migration
The test queries for '[data-testId="contentlet-thumbnail"]' but the new component uses 'dot-content-thumbnail' selector. This would cause test failures as the element cannot be found after migration to the new component.
|
|
||
| const { inode, modDate } = NEW_FILE_MOCK.entity; | ||
|
|
||
| expect(spectator.query(byTestId('contentlet-thumbnail'))).toBeTruthy(); |
There was a problem hiding this comment.
🟠 [High] Test ID mismatch in contentlet thumbnail verification
The test checks for contentlet-thumbnail test ID but the new component uses dot-content-thumbnail without this ID. Test line 150 uses deprecated selector, causing false negatives.
| `, | ||
| host: { class: 'flex h-full w-full items-center justify-center' }, | ||
| changeDetection: ChangeDetectionStrategy.OnPush | ||
| }) |
There was a problem hiding this comment.
🟠 [High] Missing accessible name for icon fallback
The $alt input defaults to empty string, resulting in empty aria-label attributes. This violates WCAG 1.1.1 by failing to provide text alternatives for non-text content when alt isn't provided. Screen reader users receive no semantic information about the icon's purpose, as confirmed by the code binding aria-label directly to an empty default alt property.
| * to the `dot-content-thumbnail` folder — consumers use the viewer. | ||
| */ | ||
| @Component({ | ||
| selector: 'dot-content-thumbnail-image', |
There was a problem hiding this comment.
🟠 [High] Cached image load events may fire before parent subscription
The image load event is triggered synchronously for cached images during ngOnInit, before parent components subscribe to stateChange. This results in the 'loaded' event being emitted but not captured, leaving thumbnails in a loading state indefinitely.
Introduce a single Angular presentational thumbnail viewer in libs/ui that replaces both the Stencil dot-contentlet-thumbnail web component and the Angular dot-temp-file-thumbnail across the whole admin app. - New dot-content-thumbnail viewer with per-type internal renderers (image/svg/pdf via img, playable/first-frame video, Material Symbols icon), a loading state machine (PrimeNG-skeleton shimmer, 300ms cross-fade reveal) and icon fallback on media error. - Consumers pass the contentlet directly ([contentlet] + [options]) or a pre-resolved model ([thumbnail]); mappers replicate the legacy Stencil URL strategies (/dA, /contentAsset, pdf_page, svg raw asset) with deliberate fixes: SVG resolves before the fieldVariable path (never rasterized, renders object-contain) and image/* content types qualify as images when isImage is absent (image-editor saves). - Icon glyph auto-scales via container queries (iconSize input as override) and recolors through the --dot-content-thumbnail-icon-color CSS property. - Playable videos are never masked by the skeleton: the native element owns its loading UX; first-frame previews use a #t=0.1 media fragment instead of the legacy canvas approach. - Migrate all 10 usages: file-field preview (temp + contentlet), asset-search card, browser-selector dataview, content-drive folder list, UVE palette, block-editor contentlet block + suggestions, relationship field (list and select-existing dialog). - Delete dot-temp-file-thumbnail (single consumer, absorbed) and mark the Stencil dot-contentlet-thumbnail as deprecated (kept for dot-card-contentlet and legacy JSP). - Fix a pre-existing crash in the file-field preview info dialog when a temp file arrives without metadata (image-editor save) by synthesizing fallback metadata. Refs: #36295
oidacra
force-pushed
the
issue-36295-dot-content-thumbnail
branch
from
f32f938 to
a212a70
Compare
oidacra
changed the base branch from
issue-36363-image-editor-image-file-fields
to
main
github-actions
Bot
added
Area : Frontend
| computed, | ||
| effect, | ||
| input, | ||
| linkedSignal, |
There was a problem hiding this comment.
🔴 [Critical] Undefined linkedSignal function usage
The component imports and uses linkedSignal from '@angular/core' (line 7), but this function doesn't exist in Angular's public API. This will cause immediate runtime errors when the component initializes, as Angular cannot resolve the dependency.
| DotContextMenuComponent, | ||
| DotAddButtonComponent, | ||
| DotContentletStatusBadgeComponent | ||
| DotContentletStatusBadgeComponent, |
There was a problem hiding this comment.
🟠 [High] Angular module conflict
DotContentThumbnailComponent is added to both imports and declarations in BlockEditorModule. Angular requires components to be declared in exactly one module. Since it's imported from @dotcms/ui, it should only be in imports, not declarations. This duplication will cause Angular compiler errors.
| TagModule, | ||
| DotContentletStatusBadgeComponent | ||
| DotContentletStatusBadgeComponent, | ||
| DotContentThumbnailComponent |
There was a problem hiding this comment.
🟠 [High] Angular module configuration error
DotContentThumbnailComponent is declared in SharedModule's declarations array (core-web/libs/block-editor/src/lib/shared/shared.module.ts:30) despite being a standalone component. Standalone components should not be declared in NgModules - this will cause Angular template errors. The component should be removed from declarations array and instead imported directly in consuming components' imports.
| template: ` | ||
| <i | ||
| [style.font-size]="$size()" | ||
| [attr.aria-label]="$alt()" |
There was a problem hiding this comment.
🟠 [High] Incorrect data-testid attribute casing
The attribute data-testId uses camelCase instead of the standard lowercase data-testid, which may cause test selectors relying on the correct casing to fail.
The component shipped styled with Tailwind utilities only, but it also renders inside bundles that do not include Tailwind — the legacy binary field web component (dotcms-binary-field-builder) — where the thumbnail collapsed to a zero-height host and the whole preview card flattened. Replace every Tailwind utility (host layout, media visibility/transition, loading overlay, icon sizing) with component-scoped styles so the viewer and its renderers carry their own layout in any bundle. Behavior, class hooks for consumers (img/video selectors, --dot-content-thumbnail-icon-color) and testids are unchanged; specs updated to the new class names. Refs: #36295
| it('should render the unified thumbnail with the temp file pdf src', () => { | ||
| spectator.detectChanges(); | ||
|
|
||
| expect(spectator.query(byTestId('contentlet-thumbnail'))).toBeTruthy(); |
There was a problem hiding this comment.
🟠 [High] Incorrect test ID for content thumbnail component in test
The test uses '[data-testId="contentlet-thumbnail"]' but the new component's template uses 'dot-content-thumbnail' as the test ID. This mismatch causes the test to fail when it should pass, or pass when it should fail, leading to unreliable test results. The correct test ID should match the component's actual data-testId attribute.
| } | ||
|
|
||
| @if ($hasLoadingOverlay()) { | ||
| <div |
There was a problem hiding this comment.
🟠 [High] Incorrect data-testid attribute casing
The attribute data-testId in line 96 uses an uppercase 'I' instead of lowercase 'i', which is the standard for HTML data attributes. This typo breaks test selectors relying on data-testid, leading to failed tests.
…-height wrappers container-type: size made the thumbnail contribute zero height, collapsing any consumer whose wrapper derives its height from content — notably the file field below its 500px container breakpoint (2-column Edit Content layouts around 1496px viewport), where the whole preview card flattened to a border line. Switch the host to container-type: inline-size (no height containment) and make the icon auto-size clamp use cqi. Since inline-size alone cannot fit the glyph to short rows, restore the explicit legacy iconSize at every call site (48px file field/dataview, 72px asset card/contentlet block, 42px suggestions, 36px content drive, 32px UVE palette, 24/30px relationship); the cqi clamp remains as a sane fallback for consumers that omit it. Refs: #36295
…narrow layouts Below the preview's 500px container breakpoint the card height derives from the media's natural ratio, so a near-square video produced a much taller card than a 16:9 image next to it. Give the thumbnail a 16:9 aspect-ratio — effective only on the narrow layout, since on the wide one both axes are already definite and aspect-ratio is ignored. Refs: #36295
…youts Same narrow-layout issue as the media thumbnails: with the container at height auto, the editableAsText code preview grew with its whole text content. Apply the same 16:9 aspect-ratio cap (inert on the wide layout); the existing bottom fade already signals the truncation. Refs: #36295
| @@ -26,7 +26,8 @@ import { SuggestionsService } from './services'; | |||
| ReactiveFormsModule, | |||
There was a problem hiding this comment.
🟠 [High] Missing module export for DotContentThumbnailComponent
DotContentThumbnailComponent is declared in SharedModule's declarations array but not included in exports, preventing its use in templates of other modules that import SharedModule. This would cause Angular template compilation errors when attempting to use the component.
1 participant
Proposed Changes
dot-content-thumbnailinlibs/ui: a single Angular presentational thumbnail viewer that replaces both the Stencildot-contentlet-thumbnailweb component and the Angulardot-temp-file-thumbnailacross the admin app.stateChangeoutput.[contentlet]+ optional[options]) or a pre-resolved model ([thumbnail]). Mappers (contentletToThumbnailModel,tempFileToThumbnailModel) replicate the legacy Stencil URL strategies (/dA,/contentAsset,pdf_page, raw SVG asset).object-contain, per the issue AC);image/*content types qualify as images whenisImageis absent (image-editor saves); playable videos are never masked by the skeleton (the native element owns its loading UX); first-frame video previews use a#t=0.1media fragment instead of the legacy canvas.pdfIcon->insert_drive_file, etc.), auto-scale via container queries (iconSizeinput as exact override) and recolor through the--dot-content-thumbnail-icon-colorCSS custom property (used by the UVE palette hover).dot-temp-file-thumbnail(single consumer, absorbed) and marked the Stencildot-contentlet-thumbnailas@deprecated(still used by the Stencil-internaldot-card-contentletand legacy JSP pages).CUSTOM_ELEMENTS_SCHEMAfrom the 7 migrated components that only declared it for the web component, restoring template type-checking.Scope note: this expands issue #36295 (originally file-field-only) to a full build-and-replace of every Angular usage, per team decision. The component was named
dot-content-thumbnailinstead of the issue'sdot-file-thumbnail.QA'd live against a running instance: all media types (raster image, SVG, PDF, video, icon fallback) across Binary/Image/File fields and all 10 migrated surfaces. Also QA'd in combination with #36406 (image editor on Image/File fields): the branches are independent and merge in any order.
This PR fixes: #36295