DX-1128: docs-native Icon component and direct HeroIcon usage (drop @ably/ui/core/Icon)#3411
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7048a5b to
f52a3b3
Compare
4c74691 to
8b2ecc6
Compare
f52a3b3 to
501a8c3
Compare
8b2ecc6 to
5d78500
Compare
501a8c3 to
3707061
Compare
5d78500 to
7e2f701
Compare
3707061 to
8fec0bb
Compare
7e2f701 to
0fa1f37
Compare
8fec0bb to
7fe3b3c
Compare
0fa1f37 to
52d51ca
Compare
52d51ca to
6803b67
Compare
7fe3b3c to
b94efdf
Compare
6803b67 to
b724bbf
Compare
kennethkalmer
left a comment
There was a problem hiding this comment.
Quick browse around the site showed nothing obviously broken, but botticus picked up some things to investigate when I asked it to review and cross-reference with the original code in @ably/ui
kennethkalmer
left a comment
There was a problem hiding this comment.
Review focused on icon-name resolution after the switch to the docs-native Icon. One live regression (Footer feedback icons), plus a few sharp edges around silent null-rendering. Details inline.
…128) Vendor a local Icon that renders only Ably's own glyphs (product, social, tech, gui-prod). Heroicons are now imported directly from @heroicons/react as components at each call site, so they tree-shake instead of going through a name->component lookup that bundles the whole set. This keeps the custom Icon's responsibility narrow and matches the direction for the rest of the vendored components.
b724bbf to
6c83436
Compare
|
Thanks @kennethkalmer - the bot has done its responses but to summarise here, I've tightened up the type definition for the icons (tbh, I would like this to be as minimal as possible in future by leaning on HeroIcons more, but I'm already satisfied with the direction this PR goes in in terms of dethroning the overcomplicated Icon component) and pulled in something that was further down the stack but opened up a regression here. |
kennethkalmer
left a comment
There was a problem hiding this comment.
All five review threads addressed and verified against the pushed code:
- Footer feedback icons now hold direct Heroicon component refs (outline + solid), rendered directly — matches the codemod. (Confirmed dormant behind ENABLE_FEEDBACK, but right to fix.)
- IconName narrowed to
keyof typeof glyphs |icon-tech-${string}`` with the glyphs map closed viasatisfies— static names now typecheck against the registry. - Dev-only
console.warnon unresolved names covers the dynamic tail. - Social links carry aria-labels, so forced
aria-hiddenkeeps accessible names intact. - Comment nit fixed.
LGTM.
Part of DX-1128. Replaces
@ably/ui/core/Iconwith a docs-nativeIcon(src/components/Icon/).What
index.tsx— the docs-nativeIcon, which now renders Ably's own glyphs only (product / social / tech /gui-prod/ badge);set-unique-ids.ts(useId-keyed) rewrites gradient/mask ids per instance; null fallback for unknown names.glyphs/— those glyphs as typed.tsxforwardRef components + a generated index, produced by running@ably/ui's own SVGR pipeline (generate-icons.tsconfig) over its source SVGs. Byte-equivalent SVG output to the compiled originals — no visual change.Icon. Every former<Icon name="icon-gui-*-{outline,solid,mini,micro}">is now a direct@heroicons/reactcomponent import at the call site, so they tree-shake.@heroicons/reactpromoted to a direct dep (outline/solid/mini/micro → 24/24/20/16).Header/LanguageSelectortests updated to query by role/label (and the burger button gained anaria-label).Behaviour
No visual change intended. Glyph coverage verified (no misses); Heroicons render identically as direct components.
Stack (DX-1128)
@ably/uitokens + reset/core CSS (merged)cn+heightsutilsIconcomponent (Ably glyphs only; Heroicons imported directly)@ably/uiTailwind scan#3410–#3416 are a linear stack, each building on the previous; #3409 has merged to
main. ▶ = this PR.Testing
🤖 Generated with Claude Code