Skip to content

Fix #1303: Remove posterUrl from Video in v0.9 Angular and add to v0.10 spec#1373

Draft
josemontespg wants to merge 6 commits intogoogle:mainfrom
josemontespg:fix-1303-video-posterurl
Draft

Fix #1303: Remove posterUrl from Video in v0.9 Angular and add to v0.10 spec#1373
josemontespg wants to merge 6 commits intogoogle:mainfrom
josemontespg:fix-1303-video-posterurl

Conversation

@josemontespg
Copy link
Copy Markdown
Collaborator

@josemontespg josemontespg commented May 8, 2026

What

Removed support for the posterUrl property in the v0.9 Angular implementation of the Video component, and added it to the v0.10 specification.

Why

Similar to the placeholder property, posterUrl was supported in the Angular renderer but missing from the v0.9 spec. It was removed from the v0.9 renderer for consistency. Since it maps directly to an HTML attribute and is essential for the Video component, it was added to the v0.10 specification.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the 'posterUrl' feature from the v0.9 Angular Video component while simultaneously adding it to the v0.10 JSON specification. The review feedback highlights several instances of unnecessary blank lines left behind after the code removal, which violate the repository's style guide regarding clean code and auto-formatting. Additionally, the reviewer noted that the component's JSDoc documentation needs to be updated to reflect the removal of the poster image feature.

import {BasicCatalogComponent} from './basic-catalog-component';
import {VideoApi} from '@a2ui/web_core/v0_9/basic_catalog';
import {AnyDuringSchemaAlignment} from '../types';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This change leaves an extra blank line after the imports. Additionally, the component's JSDoc at line 25 still mentions an 'optional poster image', which is no longer supported in v0.9 after this removal. Please remove the extra blank line and update the documentation to reflect the removal of the poster image feature.

References
  1. The repository style guide (lines 9-11) requires code to follow relevant style guides and use the correct auto-formatter. Leaving extra blank lines after removing code is a violation of clean coding standards typically enforced by such formatters. (link)

[attr.src]="url() || null"
controls
[attr.poster]="posterUrl() || null"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Removing the posterUrl attribute leaves an unnecessary blank line inside the <video> tag. Please remove it to maintain clean template code and adhere to the project's style guidelines regarding formatting.

References
  1. The repository style guide (lines 9-11) requires code to follow relevant style guides and use the correct auto-formatter. Leaving extra blank lines after removing code is a violation of clean coding standards typically enforced by such formatters. (link)

readonly posterUrl = computed(() =>
(this.props() as AnyDuringSchemaAlignment)['posterUrl']?.value(),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

An extra blank line was left at the end of the class body. Please remove it to keep the code concise and consistent with the repository's coding style.

References
  1. The repository style guide (lines 9-11) requires code to follow relevant style guides and use the correct auto-formatter. Leaving extra blank lines after removing code is a violation of clean coding standards typically enforced by such formatters. (link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant