chore: converted mixins to controllers and refactored badge#6130
chore: converted mixins to controllers and refactored badge#6130caseyisonit wants to merge 4 commits intomainfrom
Conversation
|
caseyisonit
added
Status:Ready for review
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
|
What is the win here? We don't have any gates to verify this new behaviour. This is risky. |
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
I like the idea of this, but I would like a few more things before I do a more in-depth review:
- simple stories that implement these controllers
- documentation for these controllers in a controllers section of storyboook
- tests specifically for the controllers
@Rajdeepc the win is that these should have always been controllers not mixins, this dramatically reduces the complexity of API inheritence and follows the code style guidelines on controllers. There is little to no risk to this at all at this point in our migration. Badge is still working as expected and i will implement @nikkimk feedback for docs and tests. |
There was a problem hiding this comment.
Clean implementation and code quality seems really nice but its not merge ready until there is some tests and changeset.
A few feedback:
- +1 to @nikkimk 's review request.
- We removed and added new public exports so we need changeset here.
| if (changed) { | ||
| this.host.requestUpdate(); | ||
| } |
There was a problem hiding this comment.
The old pattern was that we deferred the re-render through this.updateComplete.then(() => this.requestUpdate()): but now this is synchronous. In practice this may be fine because Lit batches requestUpdate() calls, but it's a behavioural change that should be explicitly noted and tested.
| protected get slotHasContent(): boolean { | ||
| return this.slotText.hasContent; | ||
| } |
There was a problem hiding this comment.
This is dead code. Please get clarification if 2nd-gen Badge component should support icon-only semantics or not or else this is a missing feature. Let's clarify this first.
| } | ||
| return node.textContent ? node.textContent.trim().length > 0 : false; | ||
| }); | ||
| this._hasContent = relevant.length > 0; |
There was a problem hiding this comment.
Since this.slothasContent was a reactive property in update() it inherently was triggering Lit update cycle but now if initial content is present when hostConnected fires, the host won't get a re-render to reflect that state unless a subsequent render is already scheduled. This is likely fine during initial connection (a render is pending), but please verify with tests
caseyisonit
added
Status:Addressing feedback
5 participants
Description
Converts the
ObserveSlotPresenceandObserveSlotTextmixins in the 2nd-gen core package into reactive controllers (SlotPresenceControllerandSlotTextController), and updatesBadgeBaseas the first consumer of the new pattern.This follows the mixin composition guide recommendation to prefer controllers over deeply nested mixin chains.
What changed
New controllers (
2nd-gen/packages/core/controllers/)SlotPresenceController— observes whether slotted content matching CSS selectors is present in the host's light DOM. ReplacesObserveSlotPresencemixin.SlotTextController— observes whether a slot contains meaningful text or element content. Replaces -ObserveSlotTextmixin. Requires binding handleSlotChange to the slot's@slotchangeevent in the template.Deleted mixins (
2nd-gen/packages/core/mixins/)observe-slot-presence.ts— removed; functionality moved toSlotPresenceController.observe-slot-text.ts— removed; functionality moved toSlotTextController.mixins/index.ts.Updated components
Badge.base.ts— replaced triple-mixin inheritance(SizedMixin(ObserveSlotText(ObserveSlotPresence(...))))withSizedMixin(SpectrumElement)plus two controller instances (slotPresence,slotText). AddedhasIconandslotHasContentgetter wrappers.Badge.ts (SWC)— added@slotchange=${this.slotText.handleSlotChange}to the default slot.Documentation
MIGRATION.mdandREADME.mdto reflect the new controller locations and remove mixin references.Motivation and context
BadgeBasepreviously used a 3-level mixin chain:SizedMixin(ObserveSlotText(ObserveSlotPresence(SpectrumElement))). This violated the project's mixin composition guideline (max 2 levels) and made the class hierarchy harder to reason about. Reactive controllers are the idiomatic Lit pattern for cross-cutting concerns — they compose without deepening the prototype chain, are easier to test in isolation, and can be added or removed without restructuring the class hierarchy.Related issue(s)
fixes [Issue Number]
Screenshots (if appropriate)
N/A — no visual changes. The controllers reproduce the same behavior as the mixins they replace.
Author's checklist
Reviewer's checklist
Manual review test cases
Testing for this change relies on CI passing. The controllers reproduce identical behavior to the mixins, so existing Badge tests validate correctness.
CI pipeline passes (build, lint, unit tests, VRTs)
Badge renders icon-only, text-only, and icon+text variants correctly
No remaining imports of ObserveSlotPresence or ObserveSlotText in 2nd-gen/