Feature: Add ankh-icon component with Material Symbols font rendering#10
Closed
Feature: Add ankh-icon component with Material Symbols font rendering#10
Conversation
|
Task linked: CU-86af6pbbr Implement ankh-icon component |
Review Summary by QodoAdd ankh-icon component with Material Symbols font rendering
WalkthroughsDescription• Add ankh-icon component rendering Material Symbols via font ligatures • Implement composable CSS custom properties for size and fill axes • Include 22 unit tests and 7 axe-core accessibility compliance tests • Document icon rendering strategy in ADR-002 with design rationale • Add jsdom and vitest-axe dev dependencies for testing Diagramflowchart LR
A["ankh-icon Component"] --> B["Props: name, size, filled, label"]
B --> C["Light DOM Rendering"]
C --> D["Material Symbols Font"]
D --> E["Ligature Resolution"]
A --> F["CSS Styling"]
F --> G["Private Custom Properties"]
G --> H["Composable Size & Fill"]
A --> I["Accessibility"]
I --> J["Decorative or Meaningful Icons"]
J --> K["WCAG Compliance"]
File Changes1. src/components/ankh-icon/ankh-icon.tsx
|
Code Review by Qodo
1. SemVer impact not documented
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+9
to
+41
| @Component({ | ||
| tag: 'ankh-icon', | ||
| styleUrl: 'ankh-icon.css', | ||
| shadow: false, | ||
| }) | ||
| export class AnkhIcon { | ||
| /** | ||
| * Icon name from Material Symbols (e.g. "home", "settings", "favorite"). | ||
| * Uses ligature resolution — the name is rendered as text content and | ||
| * the Material Symbols font converts it to the corresponding glyph. | ||
| */ | ||
| @Prop() name!: string; | ||
|
|
||
| /** | ||
| * The rendered size of the icon. | ||
| * Maps to --icon-size-sm/md/lg/xl tokens with optical size tracking. | ||
| * @default 'md' | ||
| */ | ||
| @Prop() size: IconSize = 'md'; | ||
|
|
||
| /** | ||
| * Whether to render the filled variant of the icon. | ||
| * Controls the FILL axis of the Material Symbols variable font. | ||
| * @default false | ||
| */ | ||
| @Prop() filled: boolean = false; | ||
|
|
||
| /** | ||
| * Accessible label for meaningful icons. | ||
| * When provided: sets role="img" and aria-label. | ||
| * When omitted: icon is treated as decorative (aria-hidden="true"). | ||
| */ | ||
| @Prop() label?: string; |
There was a problem hiding this comment.
1. Semver impact not documented 📘 Rule violation ✓ Correctness
• The PR introduces a new public component API (<ankh-icon> with new props/types), but the PR description does not state the SemVer impact (patch/minor/major). • This makes it harder for consumers to understand the release implications of the new public API surface.
Agent Prompt
## Issue description
A new public component API (`ankh-icon`) was added, but the PR does not document the SemVer impact (patch/minor/major).
## Issue Context
The repository PR template does not prompt authors for SemVer impact, so public API changes can be merged without consumers knowing release implications.
## Fix Focus Areas
- .github/PULL_REQUEST_TEMPLATE.md[1-11]
- README.md[90-95]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+59
| @Component({ | ||
| tag: 'ankh-icon', | ||
| styleUrl: 'ankh-icon.css', | ||
| shadow: false, | ||
| }) | ||
| export class AnkhIcon { | ||
| /** | ||
| * Icon name from Material Symbols (e.g. "home", "settings", "favorite"). | ||
| * Uses ligature resolution — the name is rendered as text content and | ||
| * the Material Symbols font converts it to the corresponding glyph. | ||
| */ | ||
| @Prop() name!: string; | ||
|
|
||
| /** | ||
| * The rendered size of the icon. | ||
| * Maps to --icon-size-sm/md/lg/xl tokens with optical size tracking. | ||
| * @default 'md' | ||
| */ | ||
| @Prop() size: IconSize = 'md'; | ||
|
|
||
| /** | ||
| * Whether to render the filled variant of the icon. | ||
| * Controls the FILL axis of the Material Symbols variable font. | ||
| * @default false | ||
| */ | ||
| @Prop() filled: boolean = false; | ||
|
|
||
| /** | ||
| * Accessible label for meaningful icons. | ||
| * When provided: sets role="img" and aria-label. | ||
| * When omitted: icon is treated as decorative (aria-hidden="true"). | ||
| */ | ||
| @Prop() label?: string; | ||
|
|
||
| render() { | ||
| const isDecorative = !this.label; | ||
|
|
||
| return ( | ||
| <Host class="ankh-icon-host"> | ||
| <span | ||
| class={cn('ankh-icon', 'material-symbols-outlined', `ankh-icon--${this.size}`, this.filled && 'ankh-icon--filled')} | ||
| role={isDecorative ? undefined : 'img'} | ||
| aria-label={this.label || undefined} | ||
| aria-hidden={isDecorative ? 'true' : undefined} | ||
| > | ||
| {this.name} | ||
| </span> | ||
| </Host> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Root readme missing ankh-icon 📘 Rule violation ✓ Correctness
• The root README.md “Available Components” list and usage examples do not include the newly added <ankh-icon> component. • This violates the requirement to update consumer-facing docs/examples when new component behavior/variants are introduced.
Agent Prompt
## Issue description
The new `<ankh-icon>` component is not documented in the root `README.md` component list or usage section.
## Issue Context
Consumers rely on the root README for what components exist and how to use them; without adding `ankh-icon`, the new public API is effectively undiscoverable.
## Fix Focus Areas
- README.md[37-67]
- README.md[85-95]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+12
to
+22
| .ankh-icon { | ||
| /* Composable font variation axes (private custom properties) */ | ||
| --_fill: 0; | ||
| --_opsz: 24; | ||
|
|
||
| /* Material Symbols base */ | ||
| font-variation-settings: 'FILL' var(--_fill), 'wght' 400, 'GRAD' 0, 'opsz' var(--_opsz); | ||
|
|
||
| /* Default size maps to md token */ | ||
| font-size: var(--icon-size-md, 24px); | ||
|
|
There was a problem hiding this comment.
3. Missing icon font-family 🐞 Bug ✓ Correctness
• The ADR explicitly states the component sets font-family, but the implementation never sets it, so consumers who only load the font via @font-face (or don’t include Material’s CSS class definitions) can see the ligature text (e.g. "home") instead of a glyph. • The component currently relies on an external material-symbols-outlined CSS class to set the font, but this requirement is not enforced or documented by the component itself. • Impact: icons may silently fail in real consumer apps depending on how fonts/styles are loaded, and the component won’t meet the ADR’s stated contract.
Agent Prompt
## Issue description
`ankh-icon` is documented (ADR-002) as setting its own `font-family`, but the implementation never sets it. As a result, the component currently depends on external CSS for the `material-symbols-outlined` class to provide the correct font family.
## Issue Context
Consumers may load Material Symbols via `@font-face` without including Google’s helper CSS (or may not have a `.material-symbols-outlined` class available). In those setups, the icon will render as plain ligature text.
## Fix Focus Areas
- src/components/ankh-icon/ankh-icon.css[12-46]
- src/components/ankh-icon/ankh-icon.tsx[47-55]
- docs/adr/002-icon-rendering-strategy.md[43-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
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
2 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.
Summary
Add
ankh-iconcomponent that renders Material Symbols icons via ligature resolution on the variable icon fontChanges
Notes