Give buttons a shared primary/secondary/destructive style #118
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/shared-button-style"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 185. Every button in the ticket's own list — Accept/Decline, Remove/Leave/Cancel, the delete and move confirmations — carried no
classat all.PlaceFormPanel's own discard confirmation (PR #117) shipped the same defect while this ticket was open; folded in too.buttons.css(new): four weights —.btn-primary,.btn-secondary,.btn-destructive, and.btn-ghost/.btn-ghost-destructivefor the quiet link-shaped triggers..btn-primaryisforms.css's existing.auth-form buttonrule with.btn-primaryadded to its selector list, not duplicated.--colour-danger(Remove, Leave, Delete, Decline). Promote is secondary: consequential, not destructive.place-detail-edit/-delete/-moveandgroup-form-manage-members/-deletewere initially left as exemptions; review found thatplace-detail-delete's trigger sat unweighted right beside its own.btn-destructiveconfirmation — the same flow, one styled and one not, which is precisely the "two systems standing" the ticket forbade. Giving them a named weight resolves it with no visual change: onlymarginstays component-scoped, verified by diffing both stylesheets.PlaceAccordion's disclosure and list-navigation buttons remain genuine exemptions — they fill their row and must not read as standalone buttons.ButtonWeightCoverage(new architecture test): every<button>inPlaceMark.WebUImust name a weight or be exempt. Exemptions are(file, class)pairs, so a class name cannot be borrowed elsewhere to bypass weighting.GroupMemberRowno longer computes its weight. It previously setclass="@ConfirmActionCssClass", which the guard cannot see — review proved this live by silently downgrading Remove/Leave from destructive to secondary with nothing catching it. Now@if/elsebranches with literal classes, asserted by three bUnit tests.ADR-0103.
Review found four defects in the first two commits, each fixed and confirmed by mutation rather than inspection. The guard's account of its own limits was wrong twice before settling; it now hedges rather than enumerating, and a search of every
<button>'sclass=value confirms exactly one computed case remains (PlaceAccordion), matching what it claims.Suite verified from a clean
dotnet build PlaceMark.slnx: 1690 passed, 4 skipped, 1694 total, architecture tests 115. Earlier revisions of this description quoted inconsistent figures; this one is measured.Verdict: changes needed
One finding, verified by running it rather than reading it.
ButtonWeightCoverageflags a correctly weighted button as unweighted when its class attribute mixes a literal weight with a Razor expression. Confirmed by injectinginto
PlaceFormPanel.razor— the test fails on it. Neither regex handles the mixed case:_computedClassAttributeonly matches when@is the first character after the quote, and_literalClassAttribute's[^"@]*refuses any value containing@, so the button falls through with an empty class list.It fails closed, so it cannot let an unstyled button through — but
class="btn-primary @(…)"is an ordinary Blazor pattern, and the next person to write one gets an architecture-test failure telling them their weighted button is unweighted. Strip an@-expression out of the value before splitting, rather than rejecting values that contain one.Related: the class remarks name
PlaceAccordion's fully-computedclass="@PlaceButtonClass(place.Id)"as "the one case in this codebase" and characterise the gap as skip-outright. That is incomplete — partially-computed values behave differently, and are flagged rather than skipped. Worth correcting, given ADR-0066's own history of a limitation comment that was accurate against one version of the check and wrong by the next.Separately confirmed non-vacuous: a bare
<button type="button">injected intoPlaceFormPanel.razorreddens the test on the exact tag.Note on process: this is the authoring agent's own PR reviewed by the orchestrator, because the session's subagent limit is exhausted. An independent
mergeableverdict is still owed before this merges — treat the above as findings, not as a completed review.7c9531d099990ce51818Verdict: changes needed
Reviewed at
990ce51(mergeable, CI run #528 green). Independently verified theClassTokensrewrite: both of the author's own claimed cases reproduce (class="btn-primary @(…)"passes,class="foo @(…)"still fails), and I could not find a false pass in the new balanced-paren/string scanner itself despite deliberately attacking it — nested parens, parens inside a string, two expressions, an entirely-computed value, a verbatim doubled-quote string all resolve correctly. One case (@(x ? "\"" : "y"), a backslash-escaped quote) does break the scan, but fails closed (returns[], flags a real weighted button as a gap) — annoying, not a hole, and a distinct bug from the one already reported.1. The exempt list is a real, verified false pass — this is the sharpest finding. Any new button can bypass weighting entirely by reusing any of the 15 exempt class strings verbatim, regardless of what it actually does:
ButtonWeightCoverage.UnweightedButtons()reports zero gaps for this. The exempt list is matched by string equality against a bare class token, with no association to the component or purpose it was written for — it's not "this specific drawer toggle is exempt", it's "the literal textdrawer-toggleis exempt, wherever it appears." This defeats the ticket's whole point and should be closed (e.g. matching class and file/component) before merge.2. The ghost-link exemptions don't hold up against "do not leave both systems standing."
place-detail-delete(the trigger) and its ownbtn-destructiveconfirmation ("Yes, delete") sit in the same component, same flow, same action — one is on the new weight system, the other deliberately isn't. Same forplace-detail-edit/-moveandgroup-form-manage-members/-delete. This isn't the same category as the accordion disclosure/nav buttons (which have a real layout reason — filling and blending into a row); it's the identical "act on an object" control the ticket is about, in old and new clothes within one flow. The task-156 (no browser) blocker is real for actually redesigning their shape, but that argues for scoping this PR narrower or filing a explicit follow-up, not for calling the split intentional and closing the ticket on it.3.
ButtonWeightCoverage's doc comment and ADR-0103 both claimPlaceAccordionis "the one case" of an unjudgeable computed class — false as of this PR.GroupMemberRow.razor's own confirm button (class="@ConfirmActionCssClass") is a second one, added by this very PR, named nowhere. MutatedConfirmActionCssClassto always return"btn-secondary"(silently downgrading Remove/Leave from destructive) — the guard doesn't catch it (documented limit), and neither do any of the 571PlaceMark.WebUI.Tests(no bUnit assertion on this class anywhere). A genuinely destructive action's weight is unguarded, full stop.4. Two more fails-closed traps, for completeness (not blocking, but real): a single-quoted
class='btn-primary'is treated as no class at all (same family as the already-fixed bug, different trigger); a<button>inside a@* … *@comment is still matched and flagged since comments aren't stripped before scanning.5. Pre-existing, unrelated to this PR's own diff, worth knowing about: the outer
_buttonOpeningTagregex (<button\b[^>]*>, unchanged) stops at the first literal>anywhere in the tag — including the one in@onclick="() => …". Today's buttons all putclass=before any arrow-bearing attribute, so it happens to work, but it's order-dependent by accident, not by design. A future button withclass=after an@onclick="() => …"will be silently truncated and flagged as a gap regardless of its real weight. Confirmed by injection.Everything else checked out: every button the ticket named carries the right weight in markup (Accept/Promote/Cancel/Move-confirm secondary, Decline/Remove/Leave/Delete-confirm destructive), and appearance itself is correctly left unverified.
Verdict: mergeable
Reviewed
15b4e45specifically (independent of the two commits I authored). All four findings from the prior review are genuinely fixed, verified by mutation rather than by reading:GroupMemberRowdowngrade: reverted the confirm button's destructive-branch literal tobtn-secondary— both new bUnit tests (...RemoveConfirmationShown...,...LeaveConfirmationShown...) reddened on the exact class-attribute diff. The computedConfirmActionCssClassproperty is gone outright, not just untested.drawer-togglebypass: addedclass="drawer-toggle"to an unrelated button inGroupMemberRow.razor—UnweightedButtons()now correctly flags it (IsExemptInFilerequires both class and file to match).ClassTokens/MatchButtonOpeningTagsconfirm@onclick="() => …"beforeclass=no longer truncates the tag, and — the case that matters most — a bare button with the same=>hazard is still correctly flagged as ungapped rather than silently absorbing a sibling'sclass=into its own match.PlaceDetailPanel.razor.css/GroupFormPanel.razor.cssagainst990ce51line by line. Every property exceptmarginmoved intobuttons.css's.btn-ghost/.btn-ghost-destructiveverbatim (same values, same hover shape); appearance is unchanged by construction, not merely by claim.Is there a third computed-class case? Searched every
<button>insrc/PlaceMark.WebUIfor any@inside its ownclassattribute value (not a proximity grep — parsed each tag's ownclass=value directly). Exactly one:PlaceAccordion.razor'sclass="@PlaceButtonClass(place.Id)", the same one the guard's own (now appropriately hedged) remarks describe. No third case exists today, and the doc comment's refusal to name a fixed list this time is the right call given its own two-strikes history.Single-quoted class and comment-embedded buttons (items 4 in the prior review) are both exercised directly and pass.
Two things worth a line, not a blocker:
dotnet buildimmediately before quoting it, rather than repeating a stale figure.15b4e45had not completed as of this review (Status: running) — confirm it's green before merging; this verdict is on the code, not on an unconfirmed run.dotnet format --verify-no-changesandgit merge-treeagainstorigin/mainare both clean.