Skip to content

Fix #1303: Remove color prop from Icon#1371

Draft
josemontespg wants to merge 4 commits intogoogle:mainfrom
josemontespg:fix-1303-icon-color
Draft

Fix #1303: Remove color prop from Icon#1371
josemontespg wants to merge 4 commits intogoogle:mainfrom
josemontespg:fix-1303-icon-color

Conversation

@josemontespg
Copy link
Copy Markdown
Collaborator

@josemontespg josemontespg commented May 8, 2026

What

Removed support for the color property in the v0.9 Angular implementation of the Icon component.

Why

The color property was not part of the v0.9 specification, leading to inconsistency. As discussed in issue #1303, it was decided to remove it from the renderer and rely on CSS overrides (using the existing --a2ui-icon-color variable) rather than adding it to the spec, to avoid introducing colors into the spec.

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 color property and its associated inline styling from the IconComponent, along with the AnyDuringSchemaAlignment import. Feedback focuses on maintaining code cleanliness by removing extra empty lines and ensuring that tests are updated to reflect the component's API changes, in accordance with the repository's style guide.

Comment on lines 19 to +20
import {IconApi} 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 empty line between the imports and the constants. It's recommended to maintain only one empty line for better readability.

Additionally, since this PR addresses #1303, please check if AnyDuringSchemaAlignment in types.ts can now be removed or if its documentation needs updating, as it specifically references this issue.

Suggested change
import {IconApi} from '@a2ui/web_core/v0_9/basic_catalog';
import {AnyDuringSchemaAlignment} from '../types';
import {IconApi} from '@a2ui/web_core/v0_9/basic_catalog';

Comment on lines +87 to +88
export class IconComponent extends BasicCatalogComponent<typeof IconApi> {
readonly color = computed(() => (this.props() as AnyDuringSchemaAlignment)['color']?.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

The color property has been removed from the IconComponent, which is a significant change to the component's API. According to the repository style guide, all code changes should be accompanied by tests. Please ensure that the corresponding test files (e.g., icon.component.spec.ts) are updated to reflect this change and to verify that the component still renders correctly using CSS variables.

Additionally, avoid leaving an empty line at the beginning of a class definition.

Suggested change
export class IconComponent extends BasicCatalogComponent<typeof IconApi> {
readonly color = computed(() => (this.props() as AnyDuringSchemaAlignment)['color']?.value());
export class IconComponent extends BasicCatalogComponent<typeof IconApi> {
References
  1. If there are code changes, code should have tests. (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