mirror of
https://github.com/TemujinCalidius/SurvivorCore.git
synced 2026-08-14 09:02:29 +00:00
fix(builder): chooser cards were unclickable and misrouted picks
Found by an adversarial review of the Build page before it was ever run — all three reviewers independently flagged the first one. 1. The "what is this object?" cards were dead. chooserCard parented a Size=fromScale(1,1) TextButton into Theme.panel() intending an overlay, but Theme.panel() contains a UIListLayout, which lays out EVERY GuiObject child — there is no opt-out, and ZIndex does not affect layout. So the button became another list row: clicking a card's title/summary did nothing, and the oversized button spilled past the card and took the click for the card BELOW, applying the WRONG component (which also strips the previous component's attributes). This was the page's primary interaction. Fixed with Theme.panelButton() — the card itself is the button, matching the pattern OverviewPage already uses. 2. Clear did nothing on an object whose only tag was unknown to this engine, yet reported success. The page renders exactly that branch with a Clear button. BuildAdmin.clear now takes the tags to remove explicitly, and the page passes the unknown tags it just displayed — never removing an unlisted tag speculatively, since it may belong to another plugin. It also reports "nothing to clear" instead of a false success. 3. FieldRow's help text set Position inside a UIListLayout parent, so its indent was silently dropped and every help line rendered flush left. Uses padding now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
4b6eada1ca
commit
bbfefbec38
+14
-2
@@ -267,8 +267,11 @@ function BuildAdmin.applyType(instances: { Instance }, schema: any, all: { any }
|
||||
end
|
||||
|
||||
-- The honest inverse of applyType: remove every known component tag, its attributes, and the
|
||||
-- engine's `_`-prefixed stamps. Returns { cleared }.
|
||||
function BuildAdmin.clear(instances: { Instance }, all: { any }): any
|
||||
-- engine's `_`-prefixed stamps. `extraTags` removes specific additional tags by name — the page
|
||||
-- passes the unknown tags it just displayed, so its "Clear" button actually clears what the user
|
||||
-- was shown. Tags are never removed speculatively: an unlisted tag may belong to another plugin.
|
||||
-- Returns { cleared }.
|
||||
function BuildAdmin.clear(instances: { Instance }, all: { any }, extraTags: { string }?): any
|
||||
local cleared = 0
|
||||
for _, instance in instances do
|
||||
if not alive(instance) then
|
||||
@@ -282,6 +285,15 @@ function BuildAdmin.clear(instances: { Instance }, all: { any }): any
|
||||
touched = true
|
||||
end
|
||||
end
|
||||
for _, tag in (extraTags or {}) :: { string } do
|
||||
if CollectionService:HasTag(instance, tag) then
|
||||
CollectionService:RemoveTag(instance, tag)
|
||||
touched = true
|
||||
end
|
||||
end
|
||||
if instance:GetAttribute(BOUND_ATTR) ~= nil then
|
||||
touched = true
|
||||
end
|
||||
stripEngineStamps(instance)
|
||||
if touched then
|
||||
cleared += 1
|
||||
|
||||
+12
-15
@@ -49,8 +49,11 @@ local function countAdvanced(specs: { any }): number
|
||||
end
|
||||
|
||||
-- One clickable "what is this?" card. Ineligible components render dim and inert, with the reason.
|
||||
local function chooserCard(schema: any, display: any, eligible: boolean, reason: string?, onPick): Frame
|
||||
local panel = Theme.panel()
|
||||
-- The CARD ITSELF is the button when it's pickable: a full-size button parented INTO a panel would
|
||||
-- be laid out by the panel's UIListLayout as another row (no opt-out; ZIndex doesn't affect
|
||||
-- layout), leaving the card body dead and the button spilling onto the next card.
|
||||
local function chooserCard(schema: any, display: any, eligible: boolean, reason: string?, onPick): GuiObject
|
||||
local panel: any = if eligible then Theme.panelButton() else Theme.panel()
|
||||
local title = Theme.label({
|
||||
Size = UDim2.new(1, 0, 0, 20),
|
||||
Text = display.title,
|
||||
@@ -74,15 +77,7 @@ local function chooserCard(schema: any, display: any, eligible: boolean, reason:
|
||||
panel
|
||||
|
||||
if eligible then
|
||||
-- A transparent button over the whole card keeps the panel's styling but makes it clickable.
|
||||
local hit = Theme.make("TextButton", {
|
||||
Size = UDim2.fromScale(1, 1),
|
||||
BackgroundTransparency = 1,
|
||||
Text = "",
|
||||
ZIndex = 3,
|
||||
}) :: TextButton
|
||||
hit.Parent = panel
|
||||
hit.MouseButton1Click:Connect(function()
|
||||
(panel :: TextButton).MouseButton1Click:Connect(function()
|
||||
onPick(schema)
|
||||
end)
|
||||
Theme.hover(panel, Theme.COLOR.PANEL, Theme.COLOR.PANEL_HOVER)
|
||||
@@ -160,12 +155,13 @@ function BuildAdminUi.mount(container: Frame, BuildAdmin: any, ContentAdmin: any
|
||||
end
|
||||
end
|
||||
|
||||
local function clearType(instances: { Instance }, all: { any })
|
||||
local res = actions.clear(instances, all)
|
||||
local function clearType(instances: { Instance }, all: { any }, extraTags: { string }?)
|
||||
local res = actions.clear(instances, all, extraTags)
|
||||
chooserOpen = false
|
||||
refresh()
|
||||
if typeof(res) == "table" then
|
||||
say(`✓ cleared {res.cleared or 0} object(s)`)
|
||||
local n = res.cleared or 0
|
||||
say(if n > 0 then `✓ cleared {n} object(s)` else "nothing to clear on that object")
|
||||
end
|
||||
end
|
||||
|
||||
@@ -432,7 +428,8 @@ function BuildAdminUi.mount(container: Frame, BuildAdmin: any, ContentAdmin: any
|
||||
})
|
||||
clearBtn.Parent = scroll
|
||||
clearBtn.MouseButton1Click:Connect(function()
|
||||
clearType({ instance }, all)
|
||||
-- Pass the unknown tags we just listed, so Clear removes exactly what was shown.
|
||||
clearType({ instance }, all, id.unknownTags)
|
||||
end)
|
||||
end
|
||||
|
||||
|
||||
@@ -140,18 +140,20 @@ function FieldRow.build(opts: any): Frame
|
||||
end
|
||||
|
||||
if hasHelp then
|
||||
Theme.label({
|
||||
-- `row` owns a UIListLayout, which overwrites child Position — so the indent under the
|
||||
-- label column has to come from padding, not Position.
|
||||
local help = Theme.label({
|
||||
Size = UDim2.fromScale(1, 0),
|
||||
AutomaticSize = Enum.AutomaticSize.Y,
|
||||
Position = UDim2.fromOffset(22, 0),
|
||||
Text = spec.help,
|
||||
TextColor3 = Theme.COLOR.DIM,
|
||||
TextWrapped = true,
|
||||
TextSize = 11,
|
||||
TextTruncate = Enum.TextTruncate.None,
|
||||
LayoutOrder = 2,
|
||||
}).Parent =
|
||||
row
|
||||
})
|
||||
Theme.pad(22, 0, 0, 2).Parent = help
|
||||
help.Parent = row
|
||||
end
|
||||
|
||||
return row
|
||||
|
||||
@@ -171,6 +171,27 @@ function Theme.panel(): Frame
|
||||
}) :: Frame
|
||||
end
|
||||
|
||||
-- A CLICKABLE panel: identical styling to Theme.panel, but the card itself is the button.
|
||||
-- Use this instead of parenting a full-size button "overlay" into a panel — the panel's own
|
||||
-- UIListLayout lays out EVERY GuiObject child (there is no opt-out, and ZIndex doesn't change
|
||||
-- layout), so such an "overlay" becomes another list row: the card body stops being clickable and
|
||||
-- the oversized button spills onto whatever is rendered next.
|
||||
function Theme.panelButton(): TextButton
|
||||
return Theme.make("TextButton", {
|
||||
BackgroundColor3 = Theme.COLOR.PANEL,
|
||||
BorderSizePixel = 0,
|
||||
Size = UDim2.fromScale(1, 0),
|
||||
AutomaticSize = Enum.AutomaticSize.Y,
|
||||
Text = "",
|
||||
AutoButtonColor = false, -- Theme.hover owns the highlight, matching the other cards
|
||||
}, {
|
||||
Theme.corner(8),
|
||||
Theme.stroke(),
|
||||
Theme.pad(8, 8, 6, 8),
|
||||
Theme.make("UIListLayout", { Padding = UDim.new(0, 2), SortOrder = Enum.SortOrder.LayoutOrder }),
|
||||
}) :: TextButton
|
||||
end
|
||||
|
||||
-- A small count/status pill (rail badges, entry badges).
|
||||
function Theme.badge(text: string, textColor: Color3?): TextLabel
|
||||
return Theme.make("TextLabel", {
|
||||
|
||||
@@ -123,9 +123,9 @@ local buildActions = {
|
||||
return BuildAdmin.applyType(instances, schema, all)
|
||||
end)
|
||||
end,
|
||||
clear = function(instances: { Instance }, all: { any }): any
|
||||
clear = function(instances: { Instance }, all: { any }, extraTags: { string }?): any
|
||||
return record("Build: clear component", function()
|
||||
return BuildAdmin.clear(instances, all)
|
||||
return BuildAdmin.clear(instances, all, extraTags)
|
||||
end)
|
||||
end,
|
||||
setField = function(instance: Instance, spec: any, raw: any): any
|
||||
|
||||
Reference in New Issue
Block a user