refactor(kol-link): component to use skeleton pattern - #10110
Conversation
Agent-Logs-Url: https://github.com/public-ui/kolibri/sessions/c3485d7a-5862-4a5d-ae7e-f49f66f6e2e3 Co-authored-by: deleonio <6279703+deleonio@users.noreply.github.com>
|
🚀 MCP preview deployed to Vercel: https://kolibri-4n5ri6yop-public-ui-kolibri-mcp.vercel.app |
|
🚀 Preview deployed to GitHub Pages: https://public-ui.github.io/kolibri/pr-10110/265a1c5/ |
|
Netlify Draft Deployment |
There was a problem hiding this comment.
Code Review
This pull request refactors the KolLinkWc component to utilize a new controller-based architecture and internal functional components, centralizing property definitions and validation logic. The review feedback highlights several critical issues in the new property normalizers—specifically for ariaCurrentValue, ariaExpanded, linkCallbacks, linkLabel, and tooltipAlign—where null or undefined inputs currently trigger errors instead of returning default values. Additionally, a regression in type safety was identified, as specific schema types for public props were replaced with generic strings, reducing IDE support and documentation for end-users.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the kol-link component by merging the kol-link-wc implementation into a unified public component and migrating logic to a functional component and controller structure. All dependent components and snapshots have been updated to reflect this change, and a new suite of internal prop definitions has been introduced. Review feedback identifies that the regular expressions for customClass and variant are too restrictive for short names, and an accessibility issue occurs when hideLabel is used without a string label. Further improvements were suggested regarding the redundant default tabIndex, the use of the deprecated onKeyPress event, and the limited range of allowed ARIA roles.
14781c4 to
ff32431
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the 'FC-First Composition' architectural rule, which mandates the use of Functional Components instead of KoliBri web component tags within render methods to reduce browser overhead and lifecycle complexity. The refactoring introduces a new LinkFC and LinkController, which are integrated across several components including Breadcrumb, Form, Nav, and Toolbar. Feedback identifies a missing property mapping in the link controller initialization utility, incorrect heading numbering in the migration guide, and broken markdown syntax in the architectural documentation table.
There was a problem hiding this comment.
Pull request overview
Refactors kol-link to the BaseWebComponent/Controller/FC skeleton pattern by extracting business logic into LinkController, the render into a LinkFC functional component, and per-prop configuration into many new prop-definition modules. All previous consumers of the internal <kol-link-wc> tag (nav, breadcrumb, skip-nav, toolbar, tree-item, table-stateless, form, link-button) are migrated to instantiate LinkController locally and render with LinkFC. The internal KolLinkWcTag element is removed.
Changes:
- Introduce
LinkController,LinkFC, andLinkApi/linkPropsConfigfor the new skeleton-based link. - Add a large set of per-prop definition modules under
internal/props/and export them from the props barrel. - Migrate all in-repo consumers of
<kol-link-wc>toLinkFC+ locally-managedLinkControllerinstances, and update snapshots accordingly.
Reviewed changes
Copilot reviewed 50 out of 81 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/functional-components/link/controller.ts |
New LinkController, createLinkStateAccess, click/tooltip wiring, location subscription. |
internal/functional-components/link/component.tsx |
New LinkFC rendering the anchor + tooltip. |
internal/functional-components/link/api.tsx |
New LinkApi and linkPropsConfig. |
components/link/component.tsx |
KolLink now extends BaseWebComponent<LinkApi>; watchers delegate to controller; validateAccessAndShortKey no longer called. |
components/link-button/shadow.tsx |
Migrated to LinkController; click handler diverges from controller's handleAnchorClick. |
components/nav/shadow.tsx |
Per-entry LinkController map keyed by entry object; uses LinkFC. |
components/toolbar/shadow.tsx |
Lazy controller creation inside render; _variant: 'normal' no longer reaches the controller. |
components/tree-item/component.tsx, breadcrumb/shadow.tsx, skip-nav/shadow.tsx, form/shadow.tsx, table-stateless/component.tsx |
Migrated from KolLinkWcTag to LinkFC + local LinkController. |
internal/props/*.ts (many new files) |
New per-prop definitions (access-key, aria-*, custom-class, disabled, download, inline, link-callbacks, link-label, link-role, short-key, tab-index, target, tooltip-align, variant). |
internal/props/index.ts |
Re-exports the new prop modules. |
Snapshots under components/*/__snapshots__/ |
Updated to reflect rendering through LinkFC (e.g. missing tabindex="0", missing kol-link--normal). |
Comments suppressed due to low confidence (2)
packages/components/src/internal/functional-components/link/controller.ts:89
handleAnchorClicknow returnsshouldDispatchKolEvent: event.defaultPrevented === false, andkol-link's click handler only dispatchesKolEvent.clickwhen that flag is true. Previously,KolEvent.clickwas always dispatched when the component was not disabled, regardless of whether theonClickcallback (or other listeners) calledevent.preventDefault(). This is a behavioral change for consumers that listen forKolEvent.clickwhile also providing anonClickcallback that may callpreventDefault()(e.g. to suppress navigation). Additionally,kol-link-buttonhas its own click handler inshadow.tsxthat does not perform thedefaultPreventedcheck, so the two components now diverge in behavior. Please confirm this change is intentional and align the two handlers.
packages/components/src/components/toolbar/shadow.tsx:82initLinkControllerFromPropsis called with the raw toolbar item (element) but the surrounding code overrides_variant: 'normal'and_tabIndex: tabIndexon thepropsobject that previously was spread ontoKolLinkWcTag. Those overrides are now lost on the controller:_variantis taken from the original item (likely undefined) instead of'normal', andtabIndexfrom the prop bag is unused for link items (only the explicittabIndex={tabIndex}prop onLinkFCties them together). The previousKolLinkWcTag {...props} {...element}spread set_variant="normal"on the rendered link. After this change, thekol-link--normalmodifier class is no longer added; the snapshot now only showsclass="button kol-link kol-link--inline kol-toolbar__item normal"(the literal "normal" from the host class, notkol-link--normal). If the styling of normal toolbar link items relies on.kol-link--normal, this is a visual regression.
if (!ctrl) {
ctrl = new LinkController(createLinkStateAccess(this.forceRender));
initLinkControllerFromProps(ctrl, element as { _href: string } & Partial<Record<string, unknown>>);
this.toolbarLinkCtrls.set(index, ctrl);
}
const linkCtrl = ctrl;
225429c to
28317b9
Compare
Visual Test Failures — ExpectedThe Root cause: Specific changes visible in desy/kern diffs:
These are improvements, not regressions — the theme CSS was already written to style Updated snapshots were committed in To resolve after merge: run the Generated by Claude Code |
table styles imports centralized
alert fix
There was a problem hiding this comment.
Es werden die leeren Icon tags gerendert bei den Navi entries ohne Iccons. Dadurch verschieben sich diese um das gap nach rechts. Siehe zB " 2 Navigation point"
There was a problem hiding this comment.
Beim einklappen bleibt bei Navipunkten die ein a-tag sind das label stehen.
There was a problem hiding this comment.
Das Icon des Smartbuttons fehlt hier komlett. Statt der class für das Icon wird undefined ausgegeben.
d460ace to
82e15bf
Compare
…link-skeleton-W6i6v
toolbar style refactoring
…link-skeleton-W6i6v
Summary
Refactored the
kol-linkcomponent to follow the controller pattern used by other KoliBri components, moving business logic from the component class into a dedicatedLinkControllerand extracting the render logic into a functional component (LinkFC).Key Changes
Component Architecture: Migrated
KolLinkWcfrom a monolithic component to extendBaseWebComponent<LinkApi>, delegating logic toLinkControllerLinkControllerto handle all prop validation, state management, and lifecycle hooksLinkFCfunctional component for rendering the anchor element and tooltipProps System: Extracted link-specific prop definitions into dedicated modules
access-key.ts,aria-controls.ts,aria-current-value.ts,aria-description.ts,aria-expanded.ts,aria-owns.ts,custom-class.ts,disabled.ts,download.ts,inline.ts,link-callbacks.ts,link-label.ts,link-role.ts,short-key.ts,tab-index.ts,target.ts,tooltip-align.ts,variant.tslinkPropsConfigto define the component's API surfaceAPI Definition: Introduced
LinkApitype that defines the component's public interface including methods (focus,click), refs, and stateTooltip Management: Moved tooltip controller integration into
LinkController, simplifying the component's render methodEvent Handling: Simplified click handler (
handleAnchorClick) by delegating validation and state checks to the controllerLifecycle: Consolidated all prop validation into
componentWillLoadvia the controller, removing individual watch methods from the componentImplementation Details
getRenderProp()to access validated, normalized prop values from the controllersetAnchorRef()andgetAnchorRef()ariaCurrentstate is managed by the component but updated by the controller based on location changeshideTooltip()methodwatch*instead ofvalidate*)https://claude.ai/code/session_01DBHvCfXeuCiPnhiim3k4PZ