refactor: Extract repeated code patterns throughout the code base into helpers - #2346
Conversation
…pers - Add createTimer() (internals/timing.ts) — restartable timeout; adopt in the alert mixin, slider thumb labels and chat-input typing state - Add pointToFraction() and normalizedTextContent() to utils/dom; adopt in rating and select - Add SlotController.getAssignedText(); adopt in select-group and dropdown-group label sync - Add isKey() to key-bindings for case-insensitive key checks; adopt in chat-input, drag and resize controllers - Sweeps: setStyles in resize-container/ghost, getCenterPoint in range-slider, bindIf in resize-container, isPointInsideElement in chat-input drag-leave - Specs for all new helpers
addToggleController owns the cancelable -ing event -> transition -> updateComplete -> -ed event sequence, with a host transition hook for animation ordering. - Re-base the combo-box mixin on it (select, dropdown, combo and the date pickers inherit); keep public show/hide/toggle wrappers async and internal delegation plain to preserve microtask timing - Migrate dialog, nav-drawer, banner and expansion-panel - Accordion drives the panel's emitting controller instead of re-implementing its event sequence
…vigation addRovingFocusController implements Home/End jumps and wrapping, RTL-aware arrow navigation over a flat list of enabled items, delegating focus and activation to host callbacks on top of the key-bindings controller. - Migrate tabs, stepper and radio to it - Axis predicates keep orientation-gated arrows bound but inert (stepper), and a missingCurrent policy preserves navigation from unfocused state (tabs wraps in from the edge, stepper no-ops)
createGroupRegistry keeps (scope, key)-identified groups per component family: membership lifecycle on connect/disconnect/key change, DOM-ordered member reads that skip stale entries, and derived group state synced to every member. radio/controller.ts becomes a thin configuration over it (key = name, state = reachable selection); radio.ts and its validators are untouched.
addContextProvider wraps @lit/context's ContextProvider with the recurring republish wiring: publish on host connect, republish when a watched host property changes, publish() for internal-state triggers, and force-notification so identity-stable context values still reach subscribers. Migrate theme-provider to it.
Eleven consumers move their property-derived ARIA into the internals controller's aria() option, deleting the willUpdate/update overrides, setter pushes and @watch bodies that carried it. Icon and the slot-driven group labels stay manual - they are not update-driven. Also drops divider's incorrect 'vertical' initial orientation.
Added @coercedProperty<T, Host>({ transform, onChange }) which
composes under @Property, replacing hand-written backing-field accessor
pairs with a declarative coerce/side-effect configuration. Callbacks
receive a single typed context object ({ value, host, previous }); Host
defaults to object so host-free configs pass only the value type.
The transform also covers the field initializer while onChange skips
it, preserving backing-field default semantics; Lit wraps the installed
accessor so change detection and reflection are unchanged. Rating
adopts it for max/step/single; value keeps its accessor pair since it
stores into the form-value state.
…rops pattern/minLength/maxLength/min/max/step become @coercedProperty fields sharing a single static revalidation config, dropping six backing fields. Optional properties carry explicit `= undefined` initializers so construction is marked and the first real set still triggers validation. value keeps its accessor pair since it stores into the form-value state.
Progress, select, select-group, slider-base and the predefined ranges area move their watchers into explicit willUpdate blocks; waitUntilFirstUpdate maps to a hasUpdated guard and the slider's five stacked watchers collapse into one constraint check. The watch decorator is deleted - no imports remain.
Tree, file-input, carousel, combo and the calendar base drop their hand-written locale/resourceStrings pairs and controller fields for the mixin; combo keeps its extra locale side effect via an accessor override, and the calendar base keeps its union-in/intersection-out resource typing via explicit type arguments. The Required<T> resolved-strings getter makes the remaining inline English fallbacks dead code, so they are removed.
…mixins BaseFormAssociated gains _emitTouchedEvent(touched flip + emit as one unit) and _renderValidationContainer overIgcValidationContainerComponent.create; FormAssociatedMixin gains_commitValue, which sets the touched state before the value assignmentso the setter's validation pass applies invalid styling in the samecycle. Textarea adopts both as the first consumer.
…ives Input and the date-picker change handler move to _commitValue; mask-input, the mask-behavior default input emit, slider, select and date-time-input move to _emitTouchedEvent; radio, checkbox, select, combo and the picker base render helper text through _renderValidationContainer. File-input, checkbox/radio commit sites and rating stay manual - their touch ordering or event details do not fit the primitives. Also repairs the I18nMixin factory: its overload signature crashed the custom-elements analyzer, replaced by a single AbstractConstructor signature over an abstract class.
Added: internals/templates/toggle-shell.ts renders the label-wrapped native input shared by the toggle controls; checkbox, switch and radio now describe only their part maps and control indicator. Part placement differences (the focused part on base vs control) ride on host-resolved part maps, and radio keeps its tabindex and absent blur handler through optional bindings.
Added: The "named slot with a default icon fallback" idiom now lives in a single template - internals/templates/slotted-icon.ts - with options for slot-level hiding, icon title, accessible label and aria-hidden control. Adopted in nine sites: combo (toggle/clear), date-picker base (clear/calendar), select (toggle), expansion-panel (indicator), carousel (previous/next) and chip (select). Interactive fallbacks (chip remove), non-icon fallbacks (tooltip, banner) and conditional fallbacks (tree-item) stay in place by design. The template carries a bare import of the icon module to satisfy the lit-analyzer element resolution; registration remains with the consuming components. Rendered DOM and the custom-elements manifest are unchanged for all affected tags.
The range slider's renderThumb override duplicated the base template wholesale; its two real variance points are now overridable seams instead: - _thumbAriaValueText(thumbId?) - protected hook for the aria-valuetext fallback when neither labels nor value formatting apply; the base omits the attribute, the range slider returns its min/max text. - handleThumbFocus - the range slider's private handleFocus renamed to an override of the base handler, so the shared template's focus binding reaches it. Rendered DOM and the custom-elements manifest are unchanged for both slider tags.
- the focus-entry sequence, with an _isValueEmpty() hook for the one genuine difference (the range's start/end emptiness check) - the programmatic value-setter guard as _applyValue(), typed through an abstract FormValue<T | null> declaration - the AM/PM designator mask read as _readAmPmFromMask() The range input's _applyMask override now defers the placeholder rule to super. Spin-delta resolution and date-part cursor navigation stay in the leaves — they differ behaviorally, not just textually. _datePartDeltas also becomes a concrete base getter overlaying the defaults with spinDelta, fixing igc-date-range-input silently ignoring its public spinDelta property. The custom-elements manifest is unchanged for both tags.
The three identical hours/minutes/seconds classes collapse into a single TimePart class over a per-part bounds/accessor lookup; the hours-only 12-hour rendering rides along as an optional display entry on its config row. The spinTimePart helper and TIME_BOUNDS constant dissolve into the class and table. The three inline wrap-clamp blocks unify on one wrapOrClamp helper built on the shared clamp/modulo math utilities. Single-cycle spins behave exactly as before; multi-cycle deltas are fixed - spinning the month by 12 with spin-loop no longer lands in the previous year, and the hour and day-of-month equivalents no longer roll the date over.
The manifest analyzer resolves the superclass of an extends clause from the first mixin argument, so I18nMixin(config, base) recorded the config constant as the superclass and severed the inheritance chain - dropping every inherited public property of igc-combo and igc-file-input from the generated stories. I18nMixin now takes the base class first. The explicit `= undefined` initializers the coerced-property contract requires were recorded as default: "undefined" and rendered as literal 'undefined'/NaN story args - a new manifest plugin strips them. The story generator also learned to treat the Keys<T> keyof alias as a string control, restoring the combo valueKey property it dropped.
- toCalendarDayOrNull and truncateTime in internals/date/model.ts replace the null-guarded CalendarDay round-trips in the range picker's input handlers, convertToDateRange and the calendar base value/activeDate setters. - _setCalendarActiveDate on the picker base points the calendar at the first defined candidate date, keeping the current active date when none is - the assignment both pickers kept respelling. - The i18n controller now resolves the locale-default date-time display and input formats itself, cached per locale and invalidated on resource changes. The date editors' and range picker's hand-rolled format caches (and the getDefaultDateTimeFormat free function) dissolve into it.
- toggle: drop the empty lifecycle stub, the no-op controller registration and the redundant host constraint; omit falsy keys from the emitted event init so a plain event keeps its detail-only payload - validators: extract the shared bound check behind the min/max date-time factories and share one range-ends resolver - progress: collapse the three willUpdate branches into idempotent clamps - range-slider: name the other thumb instead of looping over both to skip one - divider, tooltip: drop a coercion-only property wrapper, share the delay config - drag: finish the setStyles adoption - i18n: name and type the locale date-time format cache Comments that explained the refactoring rather than the code are trimmed throughout.
There was a problem hiding this comment.
Pull request overview
This pull request refactors repeated component behavior into reusable controllers, mixins, decorators, templates, and utilities while consolidating date-picker logic and updating Storybook and manifest generation.
Changes:
- Adds shared timing, DOM, validation, i18n, focus, grouping, toggle, ARIA, context, and form infrastructure.
- Migrates components and date editors to shared abstractions.
- Updates story metadata and fixes date-time empty-mask handling.
Four critical toggle and animation findings remain unresolved.
Reviewed changes
Copilot reviewed 118 out of 118 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Review result |
|---|---|
stories/splitter.stories.ts |
Reviewed; no final comment. |
stories/slider.stories.ts |
Reviewed; no final comment. |
stories/range-slider.stories.ts |
Reviewed; no final comment. |
stories/nav-drawer.stories.ts |
Reviewed; no final comment. |
stories/highlight.stories.ts |
Reviewed; no final comment. |
stories/file-input.stories.ts |
Reviewed; no final comment. |
stories/calendar.stories.ts |
Reviewed; no final comment. |
src/theming/theming-controller.ts |
Reviewed; no final comment. |
src/internals/validators.ts |
Reviewed; no final comment. |
src/internals/utils/types.ts |
Reviewed; no final comment. |
src/internals/utils/math.ts |
Reviewed; no final comment. |
src/internals/utils/dom.ts |
Reviewed; no final comment. |
src/internals/utils/dom.spec.ts |
Reviewed; no final comment. |
src/internals/utils/arrays.ts |
Reviewed; no final comment. |
src/internals/timing.ts |
Reviewed; no final comment. |
src/internals/timing.spec.ts |
Reviewed; no final comment. |
src/internals/templates/toggle-shell.ts |
Reviewed; no final comment. |
src/internals/templates/slotted-icon.ts |
Reviewed; no final comment. |
src/internals/templates/masked-input.ts |
Reviewed; no final comment. |
src/internals/mixins/option.ts |
Reviewed; no final comment. |
src/internals/mixins/mask-behavior.ts |
Reviewed; no final comment. |
src/internals/mixins/i18n.ts |
Reviewed; no final comment. |
src/internals/mixins/forms/types.ts |
Reviewed; no final comment. |
src/internals/mixins/forms/associated.ts |
Reviewed; no final comment. |
src/internals/mixins/combo-box.ts |
Reviewed; no final comment. |
src/internals/mixins/alert.ts |
Reviewed; no final comment. |
src/internals/i18n/i18n-controller.ts |
Reviewed; no final comment. |
src/internals/decorators/watch.ts |
Reviewed; no final comment. |
src/internals/decorators/coerced-property.ts |
Reviewed; no final comment. |
src/internals/decorators/coerced-property.spec.ts |
Reviewed; no final comment. |
src/internals/date/model.ts |
Reviewed; no final comment. |
src/internals/date/converters.ts |
Reviewed; no final comment. |
src/internals/controllers/toggle.ts |
Critical (1 vote): In-flight transitions can make show() return early and finish closed, and stale operations can emit igcOpened after cancellation. Track the in-flight target and operation generation. |
src/internals/controllers/toggle.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/slot.ts |
Reviewed; no final comment. |
src/internals/controllers/slot.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/roving-focus.ts |
Reviewed; no final comment. |
src/internals/controllers/roving-focus.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/key-bindings.ts |
Reviewed; no final comment. |
src/internals/controllers/internals.ts |
Reviewed; no final comment. |
src/internals/controllers/internals.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/group.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/drag.ts |
Reviewed; no final comment. |
src/internals/controllers/context-provider.ts |
Reviewed; no final comment. |
src/internals/controllers/context-provider.spec.ts |
Reviewed; no final comment. |
src/internals/controllers/aria-projection.ts |
Reviewed; no final comment. |
src/components/virtualization/engine.ts |
Reviewed; no final comment. |
src/components/validation-container/validation-container.ts |
Reviewed; no final comment. |
src/components/tree/tree.ts |
Reviewed; no final comment. |
src/components/tree/tree.navigation.ts |
Reviewed; no final comment. |
src/components/tooltip/tooltip.ts |
Reviewed; no final comment. |
src/components/tile-manager/tile.ts |
Reviewed; no final comment. |
src/components/tile-manager/tile-manager.ts |
Reviewed; no final comment. |
src/components/theme-provider/theme-provider.ts |
Reviewed; no final comment. |
src/components/textarea/textarea.ts |
Reviewed; no final comment. |
src/components/tabs/tabs.ts |
Reviewed; no final comment. |
src/components/tabs/tab.ts |
Reviewed; no final comment. |
src/components/stepper/step.ts |
Reviewed; no final comment. |
src/components/splitter/splitter.ts |
Reviewed; no final comment. |
src/components/slider/slider.ts |
Reviewed; no final comment. |
src/components/slider/slider-base.ts |
Reviewed; no final comment. |
src/components/slider/range-slider.ts |
Reviewed; no final comment. |
src/components/select/select.ts |
Reviewed; no final comment. |
src/components/select/select-group.ts |
Reviewed; no final comment. |
src/components/resize-container/resize-controller.ts |
Reviewed; no final comment. |
src/components/resize-container/resize-container.ts |
Reviewed; no final comment. |
src/components/resize-container/default-ghost.ts |
Reviewed; no final comment. |
src/components/rating/rating.ts |
Reviewed; no final comment. |
src/components/radio/radio.ts |
Reviewed; no final comment. |
src/components/radio/controller.ts |
Reviewed; no final comment. |
src/components/radio-group/radio-group.ts |
Reviewed; no final comment. |
src/components/qr-code/types.ts |
Reviewed; no final comment. |
src/components/qr-code/qr-code.ts |
Reviewed; no final comment. |
src/components/progress/base.ts |
Reviewed; no final comment. |
src/components/nav-drawer/nav-drawer.ts |
Reviewed; no final comment. |
src/components/mask-input/mask-input.ts |
Reviewed; no final comment. |
src/components/input/input.ts |
Reviewed; no final comment. |
src/components/icon/registry/types.ts |
Reviewed; no final comment. |
src/components/icon/icon.spec.ts |
Reviewed; no final comment. |
src/components/highlight/highlight.ts |
Reviewed; no final comment. |
src/components/file-input/file-input.ts |
Reviewed; no final comment. |
src/components/expansion-panel/expansion-panel.ts |
Reviewed; no final comment. |
src/components/dropdown/dropdown-group.ts |
Reviewed; no final comment. |
src/components/divider/divider.ts |
Reviewed; no final comment. |
src/components/dialog/dialog.ts |
Critical (1 vote): Canceled fade-ins can report success, while canceled fade-outs still close and may leave _animating set. Propagate animation results, clear state on every exit, and close only after completion. |
src/components/date-time-input/validators.ts |
Reviewed; no final comment. |
src/components/date-time-input/datetime-mask-parser.ts |
Reviewed; no final comment. |
src/components/date-time-input/date-time-input.ts |
Reviewed; no final comment. |
src/components/date-time-input/date-time-input.spec.ts |
Reviewed; no final comment. |
src/components/date-range-picker/validators.ts |
Reviewed; no final comment. |
src/components/date-range-picker/predefined-ranges-area.ts |
Reviewed; no final comment. |
src/components/date-range-picker/date-range-picker.ts |
Reviewed; no final comment. |
src/components/date-range-picker/date-range-input.ts |
Reviewed; no final comment. |
src/components/date-picker/date-picker.ts |
Reviewed; no final comment. |
src/components/date-picker/date-picker.base.ts |
Reviewed; no final comment. |
src/components/combo/combo-item.ts |
Reviewed; no final comment. |
src/components/chip/chip.ts |
Reviewed; no final comment. |
src/components/checkbox/switch.ts |
Reviewed; no final comment. |
src/components/checkbox/checkbox.ts |
Reviewed; no final comment. |
src/components/chat/chat.ts |
Reviewed; no final comment. |
src/components/chat/chat-input.ts |
Reviewed; no final comment. |
src/components/carousel/carousel-slide.ts |
Reviewed; no final comment. |
src/components/carousel/carousel-indicator.ts |
Reviewed; no final comment. |
src/components/calendar/base.ts |
Reviewed; no final comment. |
src/components/button/button-base.ts |
Reviewed; no final comment. |
src/components/button-group/button-group.ts |
Reviewed; no final comment. |
src/components/banner/banner.ts |
Critical (1 vote): A canceled close can still set open = false and report success. Preserve the animation result and only close or report success when it is true. |
src/components/badge/badge.ts |
Reviewed; no final comment. |
src/components/avatar/avatar.ts |
Reviewed; no final comment. |
src/components/accordion/accordion.ts |
Reviewed; no final comment. |
scripts/stories.mjs |
Reviewed; no final comment. |
cem.config.mjs |
Reviewed; no final comment. |
Suppressed comments (5)
src/components/accordion/accordion.ts:191
- These calls reach into
ExpansionPanel'sprivatecontroller through bracket indexing, making an implementation detail an implicit cross-component contract. Please expose an intentional internal operation/accessor for the accordion's event-emitting open/close behavior (or provide a shared controller lookup) instead of bypassing the private boundary here.
return panel['_toggleController'].hide(true);
}
private _openPanel(panel: IgcExpansionPanelComponent): Promise<boolean> {
return panel['_toggleController'].show(true);
src/components/chat/chat-input.ts:114
- The callback reads the current delay only when the timeout fires. If
stopTypingDelayis increased while the timer is armed, the callback observes that the new deadline has not elapsed and then exits without arming another timer, leaving the typing state active indefinitely until another key event. Compute the remaining delay and restart the timer when the deadline moved out.
const delay = this._state.stopTypingDelay;
if (this._userIsTyping && this._userLastTypeTime + delay <= Date.now()) {
this._setTypingStateAndEmit(false);
}
src/components/progress/base.ts:108
_updateProgress()writes--_transition-durationfromanimationDuration, but this guard does not run when onlyanimationDurationchanges. Consequently, updating that public property leaves the rendered progress element using the old transition duration until another progress property changes; includeanimationDurationin the change set.
src/internals/i18n/i18n-controller.ts:121- After a custom
resourceStringsobject is set, the controller stores a merged object in_resourceStrings. Changinglocalerefreshes_defaultResourceStringsbut does not rebuild that merged object, so this getter continues returning defaults from the previous locale whenever custom overrides are present. Re-merge_customResourceStringswith the new defaults in the locale setter before requesting the update.
src/internals/mixins/i18n.ts:50 - This public-property documentation uses the prohibited and grammatically unclear “Gets/Sets ... used for getting language” wording. It is copied into the generated API and Storybook metadata for every component using this mixin; describe the value directly instead, such as “The locale used for language-sensitive formatting and resource strings.”
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- animations: playExclusive now cancels and starts within the caller's tick. Awaiting cancelAll first let an animation started earlier in the same tick escape the cancellation and run to completion. - toggle: guard against the in-flight target instead of the lagging host `open`, so a reopen during an exit transition is no longer rejected, and track an operation generation so a superseded operation neither emits its trailing event nor reports completion. - banner, dialog: propagate the exit animation result - an interrupted close keeps the component open instead of closing it and reporting success. - chat-input: re-arm the typing timer for the time left when stopTypingDelay grows mid-timer, instead of dropping the stop notification. - progress: animationDuration changes update the transition duration on their own, without waiting for another property to change. - i18n: a per-host locale change re-merges the custom resource strings over the newly resolved defaults, as a global resource change already did. - accordion: drive the panels through an internal _show/_hide seam instead of reaching into their private toggle controller. - docs: drop the banned Gets/Sets phrasing from the locale properties and regenerate the story metadata. Every fix carries a spec that fails when its change is reverted.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 132 out of 132 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
src/components/date-range-picker/predefined-ranges-area.ts:93
- A standalone
<igc-predefined-ranges-area use-predefined-ranges>never assignsresourceStrings, so the first update does not contain that key and_predefinedRangesremains[]; the built-in chips are therefore missing even though the controller exposes the English defaults. Initialize the ranges on the first update as well (or in the constructor), and add a standalone-defaults test.
protected override willUpdate(changedProperties: PropertyValues<this>): void {
if (changedProperties.has('resourceStrings')) {
this._predefinedRanges = getPredefinedRanges(this.resourceStrings);
}
src/components/expansion-panel/expansion-panel.ts:181
ToggleController.show()returnsfalsewhen the transition is superseded or canceled, but this wrapper ignores that result and always returnstrueafter the await. Callers using the documented return value cannot tell whethershow()actually opened the panel; return the controller promise directly (after the existing already-open guard).
src/components/expansion-panel/expansion-panel.ts:170- As with
show(), this wrapper discards the controller'sfalseresult for a canceled or superseded close and unconditionally returnstrue. This contradicts the method's documented success result; return the controller promise directly after the already-closed guard.
src/components/progress/base.ts:100 - The controller's
hostUpdate()runs before the component'swillUpdate(), so this callback reads the previousmax/valueand_labelTextbefore the normalization at lines 113–114. After a caller sets an out-of-range value or changesmax, the rendered progress can be normalized whilearia-valuemax,aria-valuenow, andaria-valuetextremain one update behind. Refresh the internals after normalization (or derive the normalized values in the callback) so the ARIA state is committed in the same update.
src/components/slider/slider-base.ts:284 - On the initial update, this guard skips normalization even when
value/lower/upperand the constraints are supplied as attributes. Because custom-element attributes are applied in markup order,<igc-slider value="100" max="50">can validate the value against the default max of 100, then set max to 50, leavingvalueat 100 and the track/ARIA state out of range after the first render. Normalize whenever the constraint set changed, including the initial update (and add a regression test for reversed attribute order).
src/components/tabs/tabs.ts:182 KeyBindingControllerdefaultspreventDefaulttotrue, but this override disables it for Enter/Space activation. Tab headers are rendered as focusablediv[role=tab], so pressing Space activates the tab and also performs the browser's scroll default action. Keep the default prevention for activation keys.
src/internals/controllers/internals.ts:165- When a reflected role changes from a non-null value to
null, this branch does nothing, so the role attribute previously written by the controller remains on the host. That leaves stale semantics in the light DOM (and can mislead tooling) even thoughElementInternals.rolehas been cleared. Remove the attribute only when it still equals_reflectedRole, preserving any author-supplied role.
src/internals/i18n/i18n-controller.ts:171 - This refresh path only runs while the controller is subscribed to
onResourceChange; a component detached during a global locale/resource change reconnects with stale_defaultResourceStringsbecausehostConnected()merely re-adds the listener. It can render the old locale until another global change occurs. Refresh the controller state during reconnection (and add a disconnect → global change → reconnect regression test).
Description
Introduce reusable internals — toggle, roving-focus, group registry and
context-provider controllers, an I18n mixin, a coerced-property decorator,
shared toggle-shell/slotted-icon templates, and common timing, DOM and
validator helpers — then adopt them across the component set. Replace the
@watchdecorator withwillUpdateoverrides, standardize id generation, andhoist shared date-editor and picker logic into their base classes.
Also fixes date-time-input committing a defaults-filled date for an emptied
mask and restores manifest inheritance and defaults for story generation.
Type of Change
Testing
new functionality and regression tests added for issues already present in the
codebase.
Checklist