Skip to content

feat(ui5-icon): add fontIcon slot for font-based icon libraries#13569

Merged
nnaydenow merged 5 commits into
mainfrom
icon-font
May 22, 2026
Merged

feat(ui5-icon): add fontIcon slot for font-based icon libraries#13569
nnaydenow merged 5 commits into
mainfrom
icon-font

Conversation

@nnaydenow
Copy link
Copy Markdown
Contributor

@nnaydenow nnaydenow commented May 21, 2026

Introduces a named slot that renders a wrapper instead of , allowing applications to use font-based icon libraries (Font Awesome, Material Icons, Bootstrap Icons, Phosphor, etc.) while keeping ui5-icon as the host for sizing, design tokens, color, and accessibility.

Usage:

<ui5-icon>
  <i slot="fontIcon" class="fa-solid fa-house"></i>
</ui5-icon>

Fixes: #8186

@ui5-webcomponents-bot
Copy link
Copy Markdown
Collaborator

ui5-webcomponents-bot commented May 21, 2026

🧹 Preview deployment cleaned up: https://pr-13569--ui5-webcomponents.netlify.app

@ui5-webcomponents-bot ui5-webcomponents-bot temporarily deployed to preview May 21, 2026 08:42 Inactive
Comment thread packages/main/src/Icon.ts Outdated
@ui5-webcomponents-bot ui5-webcomponents-bot temporarily deployed to preview May 21, 2026 11:08 Inactive
@vladitasev
Copy link
Copy Markdown
Contributor

Code Review

Nice implementation! Clean approach with minimal changes to existing code. Here are my findings:

✅ Strengths

  1. Minimal footprint - only ~35 lines in Icon.ts, 18 in template
  2. Excellent test coverage - Cypress tests cover all modes (Decorative, Image, Interactive), keyboard events (Enter, Space), and click events
  3. Accessibility handled correctly - role, aria-label, aria-hidden, tabindex all properly respect the mode property
  4. Smart CSS approach - using container queries (100cqw, 100cqh) for automatic font sizing is elegant
  5. Comprehensive demo page - testing 6 different icon libraries shows thorough validation

🔍 Potential Issues

1. Missing display: flex on span root?

The CSS adds align-items and justify-content to .ui5-icon-root:

align-items: center;
justify-content: center;

But I don't see display: flex being added. The span needs this to actually center the slotted content. Is this inherited from somewhere, or does it need to be added?

2. container-type: size on all icons

This is added to the shared .ui5-icon-root rule which affects both SVG and span rendering. Worth verifying there's no regression on existing SVG icons (especially in edge cases like icons inside flex containers or with percentage-based sizing).

3. Slot naming

Agree with your TODO - "symbol" is a bit ambiguous. My preference would be:

  • glyph - technically correct term for a font character, no conflicts with existing slots
  • fontIcon - explicit and self-describing for the use case

📝 Minor

  • No CHANGELOG entry (assuming this will be added before merge)

Verdict

LGTM with minor clarifications - solid implementation, just want to confirm the flexbox centering works as intended.

@nnaydenow
Copy link
Copy Markdown
Contributor Author

Code Review

Nice implementation! Clean approach with minimal changes to existing code. Here are my findings:

✅ Strengths

  1. Minimal footprint - only ~35 lines in Icon.ts, 18 in template
  2. Excellent test coverage - Cypress tests cover all modes (Decorative, Image, Interactive), keyboard events (Enter, Space), and click events
  3. Accessibility handled correctly - role, aria-label, aria-hidden, tabindex all properly respect the mode property
  4. Smart CSS approach - using container queries (100cqw, 100cqh) for automatic font sizing is elegant
  5. Comprehensive demo page - testing 6 different icon libraries shows thorough validation

🔍 Potential Issues

1. Missing display: flex on span root?

The CSS adds align-items and justify-content to .ui5-icon-root:

align-items: center;
justify-content: center;

But I don't see display: flex being added. The span needs this to actually center the slotted content. Is this inherited from somewhere, or does it need to be added?

2. container-type: size on all icons

This is added to the shared .ui5-icon-root rule which affects both SVG and span rendering. Worth verifying there's no regression on existing SVG icons (especially in edge cases like icons inside flex containers or with percentage-based sizing).

3. Slot naming

Agree with your TODO - "symbol" is a bit ambiguous. My preference would be:

  • glyph - technically correct term for a font character, no conflicts with existing slots
  • fontIcon - explicit and self-describing for the use case

📝 Minor

  • No CHANGELOG entry (assuming this will be added before merge)

Verdict

LGTM with minor clarifications - solid implementation, just want to confirm the flexbox centering works as intended.

Tested locally and there are no problems for now using container-type: size. Renamed the slot to fontIcon as it seems most clear.

@ui5-webcomponents-bot ui5-webcomponents-bot temporarily deployed to preview May 21, 2026 13:20 Inactive
@nnaydenow nnaydenow changed the title feat(ui5-icon): add symbol slot for font-based icon libraries feat(ui5-icon): add flatIcon slot for font-based icon libraries May 21, 2026
@nnaydenow nnaydenow changed the title feat(ui5-icon): add flatIcon slot for font-based icon libraries feat(ui5-icon): add fontIcon slot for font-based icon libraries May 22, 2026
@nnaydenow nnaydenow merged commit 3cbeac5 into main May 22, 2026
20 of 21 checks passed
@nnaydenow nnaydenow deleted the icon-font branch May 22, 2026 12:59
@ui5-webcomponents-bot ui5-webcomponents-bot temporarily deployed to preview May 22, 2026 12:59 Inactive
@ui5-webcomponents-bot
Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version v2.23.0-rc.0 🎉

The release is available on v2.23.0-rc.0

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Allow ui5-icon to support non-svg icons

5 participants