Skip to content

Cu 86af6pbca audit and improve ankh button icon slots form participation matthew van dusen#14

Closed
imagineux wants to merge 7 commits intomainfrom
CU-86af6pbca_Audit-and-improve-ankh-button-icon-slots-form-participation_Matthew-Van-Dusen
Closed

Cu 86af6pbca audit and improve ankh button icon slots form participation matthew van dusen#14
imagineux wants to merge 7 commits intomainfrom
CU-86af6pbca_Audit-and-improve-ankh-button-icon-slots-form-participation_Matthew-Van-Dusen

Conversation

@imagineux
Copy link
Contributor

Summary

Changes

Notes

@Brandon-Anubis
Copy link

@qodo-code-review
Copy link

Review Summary by Qodo

Add ankh-icon component with Material Symbols font and accessibility support

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add ankh-icon component with Material Symbols font rendering
• Implement accessibility features for decorative and meaningful icons
• Add comprehensive unit and a11y tests with vitest-axe
• Introduce typecheck workflow and improve CI/CD pipeline
• Document icon rendering strategy and versioning guidelines
Diagram
flowchart LR
  A["ankh-icon Component"] --> B["Material Symbols Font"]
  A --> C["Size Variants"]
  A --> D["Fill Variant"]
  A --> E["Accessibility"]
  E --> E1["Decorative Icons"]
  E --> E2["Meaningful Icons"]
  F["Tests"] --> F1["Unit Tests"]
  F --> F2["A11y Tests"]
  G["CI/CD"] --> G1["Typecheck Workflow"]
  G --> G2["Updated PR Template"]
Loading

Grey Divider

File Changes

1. src/components/ankh-icon/ankh-icon.tsx ✨ Enhancement +74/-0

New ankh-icon component with Material Symbols rendering

src/components/ankh-icon/ankh-icon.tsx


2. src/components/ankh-icon/ankh-icon.css ✨ Enhancement +101/-0

Styles for icon sizing and fill variants

src/components/ankh-icon/ankh-icon.css


3. src/components/ankh-icon/ankh-icon.spec.ts 🧪 Tests +232/-0

Comprehensive unit tests for ankh-icon component

src/components/ankh-icon/ankh-icon.spec.ts


View more (12)
4. src/components/ankh-icon/ankh-icon.a11y.spec.ts 🧪 Tests +75/-0

Accessibility tests for decorative and meaningful icons

src/components/ankh-icon/ankh-icon.a11y.spec.ts


5. src/components/ankh-icon/readme.md 📝 Documentation +20/-0

Auto-generated component documentation

src/components/ankh-icon/readme.md


6. src/components.d.ts ✨ Enhancement +51/-0

Add ankh-icon type definitions and exports

src/components.d.ts


7. docs/adr/002-icon-rendering-strategy.md 📝 Documentation +89/-0

Document icon rendering strategy and design decisions

docs/adr/002-icon-rendering-strategy.md


8. docs/adr/README.md 📝 Documentation +1/-0

Add reference to icon rendering ADR

docs/adr/README.md


9. README.md 📝 Documentation +41/-1

Document ankh-icon component and versioning policy

README.md


10. package.json Dependencies +6/-2

Add typecheck script and testing dependencies

package.json


11. vitest.config.ts ⚙️ Configuration changes +1/-0

Configure jsdom environment for a11y tests

vitest.config.ts


12. .github/workflows/typecheck.yml ⚙️ Configuration changes +21/-0

New workflow for TypeScript type checking

.github/workflows/typecheck.yml


13. .github/workflows/main.yml ⚙️ Configuration changes +4/-1

Add typecheck job to main workflow

.github/workflows/main.yml


14. .github/workflows/pr.yml ⚙️ Configuration changes +4/-1

Add typecheck job to PR workflow

.github/workflows/pr.yml


15. .github/PULL_REQUEST_TEMPLATE.md 📝 Documentation +9/-0

Add SemVer impact checklist to PR template

.github/PULL_REQUEST_TEMPLATE.md


Grey Divider

Qodo Logo

@imagineux imagineux closed this Feb 18, 2026
@imagineux imagineux deleted the CU-86af6pbca_Audit-and-improve-ankh-button-icon-slots-form-participation_Matthew-Van-Dusen branch February 18, 2026 05:10
@qodo-code-review
Copy link

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. SemVer impact not stated 📘 Rule violation ✓ Correctness
Description
This PR introduces a new public component (<ankh-icon>), but the PR description does not declare
the required SemVer impact (patch/minor/major/none). This makes it unclear how consumers should
version and can lead to incorrect releases.
Code

.github/PULL_REQUEST_TEMPLATE.md[R5-12]

+## SemVer Impact
+
+<!-- Which version bump does this PR require? Check one. -->
+
+- [ ] **Patch** – backwards-compatible bug fix
+- [ ] **Minor** – new feature, new component, or new public prop (backwards-compatible)
+- [ ] **Major** – breaking change to an existing public API
+- [ ] **None** – docs, CI, refactor, or other non-release change
Evidence
Compliance requires documenting SemVer impact for public API changes. The README updates show a new
public component (<ankh-icon>), but the PR description contains only the blank template sections
and does not state/check a SemVer impact.

Rule 8: SemVer impact documented
README.md[37-45]
.github/PULL_REQUEST_TEMPLATE.md[5-12]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR introduces a new public API surface (`&lt;ankh-icon&gt;`), but the PR description does not declare the SemVer impact.

## Issue Context
The repository now includes a PR template section for SemVer impact, and compliance requires that PR notes document SemVer impact for public API changes.

## Fix Focus Areas
- .github/PULL_REQUEST_TEMPLATE.md[5-12]
- README.md[37-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Warnings inside render() 🐞 Bug ⛯ Reliability
Description
ankh-icon calls console.warn() inside render() for missing/invalid props. Since render() can
run frequently, this can spam logs and introduces side effects into rendering, unlike other
components in this repo.
Code

src/components/ankh-icon/ankh-icon.tsx[R45-55]

+  render() {
+    if (!this.name) {
+      console.warn('[ankh-icon] Missing required "name" prop.');
+    }
+
+    const validSize = VALID_SIZES.includes(this.size);
+    if (!validSize) {
+      console.warn(
+        `[ankh-icon] Invalid size "${this.size}" — falling back to "md". Expected one of: ${VALID_SIZES.join(', ')}`,
+      );
+    }
Evidence
The component logs warnings during render(). Other components’ render() methods in this repo do
not emit console warnings, so this introduces an inconsistent, potentially noisy behavior that can
repeat on every render cycle when props are invalid.

src/components/ankh-icon/ankh-icon.tsx[45-55]
src/components/ankh-button/ankh-button.tsx[45-66]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ankh-icon` currently emits `console.warn()` from `render()`. Since Stencil may call `render()` multiple times per component instance, invalid props can result in repeated warnings and render-time side effects.

### Issue Context
Warnings are currently triggered for missing `name` and invalid `size`.

### Fix Focus Areas
- src/components/ankh-icon/ankh-icon.tsx[45-55]

### Implementation notes
- Prefer `componentWillLoad()` for initial validation and `@Watch(&#x27;name&#x27;)` / `@Watch(&#x27;size&#x27;)` for runtime changes.
- Consider dev-only guarding (e.g., `Build.isDev`) and/or a per-instance boolean flag to warn once.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. ARIA attrs on inner span 🐞 Bug ✓ Correctness
Description
ankh-icon applies role/aria-label/aria-hidden to an inner <span> instead of the custom
element host. This currently passes the added axe tests, but it’s inconsistent with other decorative
components and may reduce semantic robustness for assistive tech.
Code

src/components/ankh-icon/ankh-icon.tsx[R61-70]

+    return (
+      <Host class="ankh-icon-host">
+        <span
+          class={cn('ankh-icon', `ankh-icon--${size}`, this.filled && 'ankh-icon--filled')}
+          role={isDecorative ? undefined : 'img'}
+          aria-label={normalizedLabel || undefined}
+          aria-hidden={isDecorative ? 'true' : undefined}
+        >
+          {this.name}
+        </span>
Evidence
ankh-icon sets its accessibility attributes on the inner .ankh-icon span. In contrast, other
non-semantic/decorative components in this codebase hide the custom element itself via
aria-hidden="true" on <Host>, establishing an internal convention for where a11y attributes
live.

src/components/ankh-icon/ankh-icon.tsx[61-71]
src/components/ankh-focus-ring/ankh-focus-ring.tsx[66-76]
src/components/ankh-ripple/ankh-ripple.tsx[225-236]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ankh-icon` sets ARIA attributes on an internal span rather than on the `&lt;ankh-icon&gt;` host element. While this may work (and current axe tests pass), it’s inconsistent with other components in this repo that apply `aria-hidden` at the Host level.

### Issue Context
Other decorative-only components (`ankh-focus-ring`, `ankh-ripple`) set `aria-hidden=&quot;true&quot;` on `&lt;Host&gt;`.

### Fix Focus Areas
- src/components/ankh-icon/ankh-icon.tsx[61-71]
- src/components/ankh-icon/ankh-icon.spec.ts[136-214]
- src/components/ankh-icon/ankh-icon.a11y.spec.ts[28-56]

### Implementation notes
- Option A (recommended for consistency): set `aria-hidden` / `role` / `aria-label` on `&lt;Host&gt;`.
- Optionally set `aria-hidden=&quot;true&quot;` on the inner span to ensure the ligature text node is never announced independently.
- Adjust unit/a11y tests to assert attributes on the host instead of the span.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +5 to +12
## SemVer Impact

<!-- Which version bump does this PR require? Check one. -->

- [ ] **Patch** – backwards-compatible bug fix
- [ ] **Minor** – new feature, new component, or new public prop (backwards-compatible)
- [ ] **Major** – breaking change to an existing public API
- [ ] **None** – docs, CI, refactor, or other non-release change

Choose a reason for hiding this comment

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

Action required

1. Semver impact not stated 📘 Rule violation ✓ Correctness

This PR introduces a new public component (<ankh-icon>), but the PR description does not declare
the required SemVer impact (patch/minor/major/none). This makes it unclear how consumers should
version and can lead to incorrect releases.
Agent Prompt
## Issue description
The PR introduces a new public API surface (`<ankh-icon>`), but the PR description does not declare the SemVer impact.

## Issue Context
The repository now includes a PR template section for SemVer impact, and compliance requires that PR notes document SemVer impact for public API changes.

## Fix Focus Areas
- .github/PULL_REQUEST_TEMPLATE.md[5-12]
- README.md[37-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1900ee346c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

* Material Symbols icon font with token-mapped sizing
*/

@layer components {

Choose a reason for hiding this comment

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

P1 Badge Remove @layer wrapper around icon styles

Wrapping the full icon stylesheet in @layer components (src/components/ankh-icon/ankh-icon.css) lowers all of these rules below any unlayered consumer CSS in cascade order, so common app-level resets such as span { font-family: ...; line-height: ...; } can override .ankh-icon and cause ligature text to render instead of Material Symbols glyphs. This is a functional regression for consumers that already ship unlayered global styles; the component base rules should not be placed in a layer unless the entire library is consistently layered.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants