Skip to content

Fix missing export Fields type for Icon#212

Open
ryanlua wants to merge 1 commit into1ForeverHD:mainfrom
ryanlua:export-fields-type
Open

Fix missing export Fields type for Icon#212
ryanlua wants to merge 1 commit into1ForeverHD:mainfrom
ryanlua:export-fields-type

Conversation

@ryanlua
Copy link
Contributor

@ryanlua ryanlua commented Mar 2, 2026

Fixes the Icon type, which is missing properties and events such as isEnabled and selected.

Reproduction:

icon.toggled:Connect(function()
	print("Toggled")
end)

Closes #214

Before

image

After

image

Copilot AI review requested due to automatic review settings March 2, 2026 06:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the public Luau type surface for Icon so that instance properties/events (e.g. isEnabled, selected, toggled) are available to typed consumers.

Changes:

  • Extend export type Icon to include Fields (properties + signals) in its intersection type.
Comments suppressed due to low confidence (1)

src/Types.lua:464

  • Methods is already defined as an intersection with StaticFunctions (type Methods = {...} & StaticFunctions), so export type Icon = Methods & Fields & StaticFunctions redundantly intersects StaticFunctions twice. Consider removing the extra & StaticFunctions here (or removing it from Methods) to keep the type composition easier to understand.
export type Icon = Methods & Fields & StaticFunctions --typeof(setmetatable({} :: Fields, MT))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

export type Icon = Methods & StaticFunctions --typeof(setmetatable({} :: Fields, MT))
export type Icon = Methods & Fields & StaticFunctions --typeof(setmetatable({} :: Fields, MT))
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that Icon includes Fields, the event/property types in Fields become part of the public API. Several Signal generics in Fields don’t match how the library fires these events at runtime (e.g. toggled is fired with (boolean, fromSource, sourceIcon); selected/deselected are fired with (fromSource, sourceIcon); viewingStarted/viewingEnded are fired with a boolean; notified is fired with a noticeId). Please update the Fields signal type parameters to reflect the actual :Fire(...) signatures so typed consumers can receive the correct arguments.

Copilot uses AI. Check for mistakes.
@ryanlua
Copy link
Contributor Author

ryanlua commented Mar 19, 2026

Side note, but this PR is a bit iffy since I don't know well how the types work for TopbarPlus and whether they are 100% correct to begin with. This PR assumes the types are 100% correct to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix .isSelected not appearing in autocompletes

2 participants