refactor(button): align ButtonComponent with CameraComponent architecture#8870
Merged
Conversation
…ture ButtonComponent now owns its 14 properties as private fields with real setters/getters, inlining the side effects that previously lived in set_* event listeners. ButtonComponentData is reduced to a single enabled field. The system drives initializeComponentData from an explicit _properties list, adds an explicit cloneComponent (the base clone would otherwise lose every non-enabled field), and builds the enabled accessor via Component._buildAccessors. The old schema-driven vec4/rgba conversions move into the hitPadding and tint setters: falsy values pass through untouched, Color/Vec4 inputs are cloned so caller mutations do not leak, and [r, g, b, a] arrays from JSON-loaded scenes are converted. Tint setters skip the visual reapply when the value is unchanged, so value-equal writes no longer cancel an in-flight fade tween. Adds a 21-test suite modelled on the ScrollbarComponent tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Public API reportThis PR changes the public API surface (+8 / −8), per the docs' rules (@ignore / @Private / undocumented are excluded). Show API diff-AttributeSchema.type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "rgba" | "asset" | "curve" | "entity"
+AttributeSchema.type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "curve" | "entity" | "rgba"
-ButtonComponent.get hoverSpriteAsset(): Asset
+ButtonComponent.get hoverSpriteAsset(): Asset | null
-ButtonComponent.get inactiveSpriteAsset(): Asset
+ButtonComponent.get inactiveSpriteAsset(): Asset | null
-ButtonComponent.get pressedSpriteAsset(): Asset
+ButtonComponent.get pressedSpriteAsset(): Asset | null
-ButtonComponent.set hoverSpriteAsset(arg: Asset)
+ButtonComponent.set hoverSpriteAsset(arg: Asset | null)
-ButtonComponent.set inactiveSpriteAsset(arg: Asset)
+ButtonComponent.set inactiveSpriteAsset(arg: Asset | null)
-ButtonComponent.set pressedSpriteAsset(arg: Asset)
+ButtonComponent.set pressedSpriteAsset(arg: Asset | null)
-ScriptAttributes.add(name: string, args: { array: boolean; assetType: string; color: string; curves: string[]; default: any; description: string; enum: any[]; max: number; min: number; placeholder: string | string[]; precision: number; schema: any[]; size: number; step: number; title: string; type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "rgba" | "asset" | "curve" | "entity" }): void
+ScriptAttributes.add(name: string, args: { array: boolean; assetType: string; color: string; curves: string[]; default: any; description: string; enum: any[]; max: number; min: number; placeholder: string | string[]; precision: number; schema: any[]; size: number; step: number; title: string; type: "string" | "number" | "boolean" | "vec2" | "vec3" | "vec4" | "json" | "rgb" | "asset" | "curve" | "entity" | "rgba" }): voidInformational only — this never fails the build. |
The type is inferred from the initializer, matching the bare /** @Private */ style of the other directly-initialized fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors ButtonComponent to match the “CameraComponent-style” architecture used by the recent ScrollbarComponent / ScrollViewComponent migrations, moving state out of the *ComponentData bag and into private fields on the component while preserving runtime behavior and adding a dedicated test suite.
Changes:
- Migrate
ButtonComponentproperties fromButtonComponentDatainto component-owned private fields; inline previousset_*side effects into setters. - Update
ButtonComponentSystemto a minimal['enabled']schema plus explicit property initialization and an explicitcloneComponent. - Add a comprehensive
ButtonComponenttest suite covering defaults, initialization semantics, cloning, and key behavioral regressions (tints, hit padding, transition modes, entity reference resolution).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/framework/components/button/component.test.mjs | New unit tests for ButtonComponent covering initialization, behavior, and cloning. |
| src/framework/components/button/system.js | System updated to minimal schema + explicit property init list + explicit cloning. |
| src/framework/components/button/data.js | Data bag reduced to enabled only. |
| src/framework/components/button/component.js | Component refactor: internal state fields, updated setters, lifecycle/listener adjustments, tween handling cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The helper is only called from the sprite asset/frame setters, so the domain is Asset|number|null rather than *. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- createButton spreads data before forcing imageEntity, so callers cannot silently override the wired image entity - fix two comments claiming null/undefined pass-through where the code passes all falsy values through, matching legacy convertValue Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The six state sprite asset accessors were documented as bare Asset, but null is their default, numeric asset ids are what scene data ships (ImageElement.spriteAsset accepts both and ElementComponent documents it as number), and the getters return whatever was stored. Declare them Asset|number|null, matching _setTransitionValue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Numeric asset ids still work internally (scene data ships them and ImageElement.spriteAsset accepts both), but the public contract encourages Asset references, so the docs advertise Asset|null only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Applies the refactor pattern from #8693 (
ScrollbarComponent) and #8777 (ScrollViewComponent) toButtonComponent, continuing the move away from*ComponentDataproperty bags. Runtime behaviour and the public property surface are unchanged (verified against the regenerated.d.ts: all 14 properties plusenabledvia the baseComponent, and all events preserved), with one deliberate documentation correction: the six state sprite asset accessors were declared as bareAsset, butnullis their default — they are now declaredAsset|null. (Numeric asset ids still work internally — scene data ships them andImageElement.spriteAssetaccepts both — but the public contract encouragesAssetreferences, so the docs deliberately do not advertisenumber.)ButtonComponentnow owns_active,_hitPadding,_transitionMode,_hoverTint,_pressedTint,_inactiveTint,_fadeDurationand the six state sprite asset/frame fields directly. The side effects that used to live inset_*listeners are inlined into the setters (active→ visual state update;transitionMode→ cancel tween, reset previous mode's visuals, reapply; tints and sprite props → force reapply). The_setValuehelper,_toggleLifecycleListeners,_onSetActive,_onSetTransitionModeand_onSetTransitionValueare gone. TheimageEntitysetter is restyled like scrollbar'shandleEntity(resolve first, single early-out) and drops its trailingthis.data.imageEntity = guidwrites.ButtonComponentDatais reduced to a singleenabledfield.ButtonComponentSystemmatchesScrollbarComponentSystem:_schema = ['enabled'], an explicit_propertieslist drivesinitializeComponentData(guarded on!== undefinedso explicit-undefineddata does not clobber class-field defaults, per the scroll-view fix), an explicitcloneComponentis added (the default base-class clone would otherwise lose every non-enabledfield), andComponent._buildAccessors(ButtonComponent.prototype, _schema)handles theenabledaccessor.The schema previously declared
hitPaddingastype: 'vec4'and the three tints astype: 'rgba', which madeComponentSystem.convertValueclone instances and construct values from arrays. That normalization is moved into the setters so JSON-loaded scenes shipping arrays (e.g.hoverTint: [0.75, 0.75, 0.75, 1]) and callers passing shared instances continue to work. ThehitPaddingsetter passes falsy values through untouched, whichElementInput.buildHitCorners's|| ZERO_VEC4fallback relies on. Tint setters skip the visual reapply when the value is unchanged, so value-equal writes no longer cancel an in-flight fade tween.Also declares the previously-undeclared
_hasHitElementListeners,_isApplyingTint,_isApplyingSpriteand_tweenInfofields (_cancelTweennow assignsnullinstead ofdelete).One accepted behaviour nuance (same as the scrollbar/scroll-view migrations): a guid string that fails to resolve via
getEntityFromIndexis no longer stashed in the data bag for re-resolution on a later clone.Tests
Adds
test/framework/components/button/component.test.mjs(21 tests), modelled on theScrollbarComponentsuite, covering: defaults; full data round-trip; explicit-undefinedhandling; tint/hitPaddingarray conversion and clone-on-set; inactive-state tinting and event suppression;transitionModeswitching (tint reset + state sprite applied) and same-value no-op;imageEntityEntity/guid/null resolution and unsubscribe-on-reassign;cloneComponentround-trip and entity remapping;resolveDuplicatedEntityReferenceProperties.npm run lintpassesnpm test— 1770 passing, 0 failuresnpm run build:types+npm run test:typespassChecklist
🤖 Generated with Claude Code