-
Notifications
You must be signed in to change notification settings - Fork 483
feat(ui): unify content thumbnails into dot-content-thumbnail component (#36295) #36424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a212a70
1898a99
c054a11
14485e8
f6e498e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { FormsModule, ReactiveFormsModule } from '@angular/forms'; | |
| import { ButtonModule } from 'primeng/button'; | ||
| import { TagModule } from 'primeng/tag'; | ||
|
|
||
| import { DotContentletStatusBadgeComponent } from '@dotcms/ui'; | ||
| import { DotContentletStatusBadgeComponent, DotContentThumbnailComponent } from '@dotcms/ui'; | ||
|
|
||
| // Shared | ||
| import { | ||
|
|
@@ -26,7 +26,8 @@ import { SuggestionsService } from './services'; | |
| ReactiveFormsModule, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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. |
||
| ButtonModule, | ||
| TagModule, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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. |
||
| DotContentletStatusBadgeComponent | ||
| DotContentletStatusBadgeComponent, | ||
| DotContentThumbnailComponent | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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. |
||
| ], | ||
| declarations: [ | ||
| SuggestionsComponent, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,15 @@ describe('DotFileFieldPreviewComponent', () => { | |
| expect(spectator.query(byTestId('download-btn'))).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should render the unified thumbnail with the temp file pdf src', () => { | ||
| spectator.detectChanges(); | ||
|
|
||
| expect(spectator.query(byTestId('contentlet-thumbnail'))).toBeTruthy(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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. |
||
| expect( | ||
| spectator.query(byTestId('dot-content-thumbnail-image')).getAttribute('src') | ||
| ).toBe(TEMP_FILE_MOCK.thumbnailUrl); | ||
| }); | ||
|
|
||
| it('should call downloadAsset when click on the download btn', () => { | ||
| const downloadSpy = jest | ||
| .spyOn(spectator.component, 'downloadAsset') | ||
|
|
@@ -80,6 +89,17 @@ describe('DotFileFieldPreviewComponent', () => { | |
| expect(downloadSpy).toHaveBeenCalledWith(expectedUrl); | ||
| }); | ||
|
|
||
| it('should render with fallback metadata when the temp file has none (image editor save)', () => { | ||
| spectator.setInput('previewFile', { | ||
| source: 'temp', | ||
| file: { ...TEMP_FILE_MOCK, metadata: undefined } | ||
| } as unknown); | ||
| spectator.detectChanges(); | ||
|
|
||
| expect(spectator.component).toBeTruthy(); | ||
| expect(spectator.query(byTestId('metadata-title'))).toHaveText(TEMP_FILE_MOCK.fileName); | ||
| }); | ||
|
|
||
| it('should not show download button when referenceUrl is missing', () => { | ||
| spectator.setInput('previewFile', { | ||
| source: 'temp', | ||
|
|
@@ -109,9 +129,15 @@ describe('DotFileFieldPreviewComponent', () => { | |
| expect(spectator.component).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should be have a dot-contentlet-thumbnail', () => { | ||
| it('should render the unified thumbnail with the contentlet image src', () => { | ||
| spectator.detectChanges(); | ||
| expect(spectator.query('dot-contentlet-thumbnail')).toBeTruthy(); | ||
|
|
||
| const { inode, modDate } = NEW_FILE_MOCK.entity; | ||
|
|
||
| expect(spectator.query(byTestId('contentlet-thumbnail'))).toBeTruthy(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [High] Test ID mismatch in contentlet thumbnail verification The test checks for |
||
| expect( | ||
| spectator.query(byTestId('dot-content-thumbnail-image')).getAttribute('src') | ||
| ).toBe(`/dA/${inode}/asset/500w/50q?r=${modDate}`); | ||
| }); | ||
|
|
||
| it('should show proper metadata', () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 [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.