Repository navigation
fix(ComplexSelector): attached status shows the status icon on the trigger row - #9731
Conversation
…igger row Fixes #8956. With the default attached placement the field got its error edge and message but no status icon, so colour was the only cue on the field itself; Selector, MultiSelector and the text and date inputs show one. The trigger row now shows useInputStatusIcon's plain icon for attached, beside the chevron, as the tooltip placement's info-tip already sits there; a press on the plain icon opens the popup like the chevron. Tooltip placement (already on main: a focusable info-tip that holds the message and describes the trigger) and detached are unchanged. Record: component:ComplexSelector FR7 states the attached icon and the tooltip button; its verification row names both tests. Test: attached shows the status icon on the trigger row and adds no tab stop; detached shows none (fails before).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ComplexSelector · Storybook | 61.4 kB (+14 B) · 10.5 kB CSS (0) | 702 (+7) | ✓ · ✓ · ✓ |
| lab/TransferListSelector | 91.9 kB (+13 B) · 1.9 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.62 MB packed (+98 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 23 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
Approval waits on one spec decision; the code change itself looks good: the trigger shows the status glyph for attached status, and the new test fails without the fix.
The pull request adds a requirement no current spec states yet, ComplexSelector FR7's "With attached status the trigger row MUST show the status icon", so an engineering owner needs to approve or amend it, including whether the glyph sits beside the chevron (as here) or replaces it (as Selector and MultiSelector do). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Non-blocking:
- The new comment above the
useInputStatusIconcall restates what the hook already documents; keeping only the reason would read: "Withtooltipplacement, Field renders no message box. The info-tip holds the message and describes the trigger." - The description's browser tally counts a tooltip-placement field that the Audit matrix story does not have; the story goes from 0 to 3 of 10 triggers with a status glyph.
[Automated review]
| // The trigger row shows the status icon for `attached`, and for `tooltip` | ||
| // an info-tip button that holds the message, as Field renders no message | ||
| // box then. `detached` shows its icon in the message box instead. |
There was a problem hiding this comment.
Non-blocking: this repeats what useInputStatusIcon already documents, and the first sentence runs 26 words over three topics. Keeping only the reason:
// With `tooltip` placement, Field renders no message box. The info-tip
// holds the message and describes the trigger.… as in Selector Per the owner ruling (record stands: Selector and MultiSelector replace the chevron), the attached status icon and the tooltip info-tip take the chevron's place; detached keeps the chevron. FR7 says so and names the tests. Merges main (FR8 and its verification row from main). A shorter comment, per review.
There was a problem hiding this comment.
Approval waits on one spec decision, and one doc fix is needed if that decision keeps the change.
This pull request adds a requirement no current spec states: ComplexSelector FR7 now says the trigger row "MUST show the status icon in place of the chevron", and that the tooltip info-tip also takes the chevron's place. An engineering owner needs to approve or amend that sentence, including that the chevron and its open/closed turn disappear whenever a status shows. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Required change:
ComplexSelector.doc.mjsstill documents the Indicator icon asrequired: true, "Trailing chevron that rotates to reflect whether the popup is open", but this change removes it whenever an attached or tooltip status shows; give it Selector'srequired: falsewording ("Trailing chevron shown when status presentation does not replace it…").
Non-blocking:
- With a status and
isLoading, the glyph renders before the Spinner, the reverse of Selector, so the Spinner takes the chevron's former slot. - The description's Evidence section describes the earlier revision (icons beside the chevron, 4 of 10 triggers); at this head the glyph replaces the chevron on 3 of 10.
[Automated review]
| // The status icon (or the tooltip-placement info-tip) sits beside the | ||
| // chevron and keeps its size when the trigger text is long. |
There was a problem hiding this comment.
Non-blocking: the glyph no longer sits beside the chevron; it replaces it. Keeping only the constraint:
// The status glyph or info-tip keeps its size when the trigger text is long.| // popup, as a press on the trigger does. | ||
| <span {...stylex.props(styles.statusTip)}>{statusTip}</span> | ||
| ))} | ||
| {isBusy && <Spinner size="sm" aria-hidden="true" />} |
There was a problem hiding this comment.
Non-blocking: with a status and isLoading, this Spinner renders after the status glyph, so it takes the chevron's former slot. Selector renders the Spinner first and the status glyph last; moving this line above the status block matches it.
…er sits before the status glyph, as in Selector Per review: the Indicator icon anatomy row is required: false with Selector's wording, since a status glyph replaces it. With a status and isLoading, the Spinner renders before the glyph, so the glyph keeps the chevron's slot. A shorter style comment.
|
/approve-spec ebbd50c |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft, and CI runs once it is marked ready for review.
An engineering owner approved the new ComplexSelector FR7 sentence at this head, and the component doc now marks the Indicator icon optional. Nothing blocks.
Non-blocking:
- Miss: with
statusVariant="tooltip"and a status that has no message, the hook renders a plain glyph, butComplexSelector.tsxstill wraps it to stop presses. A press on that glyph, which now sits in the chevron's place, opens nothing; wrapping only when the info-tip renders (for exampleisTooltipStatus && status?.message) fixes it. - The description's Evidence section still describes the earlier revision (icons beside the chevron, 4 of 10 triggers); at this head the glyph replaces the chevron on 3 of 10.
[Automated review]
| })} | ||
| /> | ||
| {isRenderable(statusTip) && | ||
| (isTooltipStatus ? ( |
There was a problem hiding this comment.
Non-blocking (Miss): with statusVariant="tooltip" and a status that has no message, useInputStatusIcon returns the plain glyph, not the info-tip button, but this branch still wraps it to stop the press. Now that the glyph sits in the chevron's place, a press at the trigger's end opens nothing. Keying the wrapper on the info-tip rendering fixes it:
(isTooltipStatus && status?.message ? (There was a problem hiding this comment.
Approved pending CI: the required build, test and docsite-test suites are deferred on this head and run before merge.
The new ComplexSelector FR7 sentence is approved by an engineering owner at this head, and the attached status glyph and the tooltip info-tip now take the chevron's place, as in Selector and MultiSelector. Nothing blocks.
Non-blocking:
- Miss (non-blocking): with
statusVariant="tooltip"and a status that has no message,useInputStatusIconreturns the plain glyph, butComplexSelector.tsx:930still wraps it to stop the press, so a press at the trigger's end opens nothing; wrapping only when the info-tip renders (for exampleisTooltipStatus && status?.message) fixes it.
[Automated review]
|
/approve-spec 7b85bef |
|
/approve-spec 9d5d7c2 |
There was a problem hiding this comment.
Approved pending CI: the full CI suites are still running on this head.
The new ComplexSelector FR7 sentence is approved by an engineering owner at this head, and the attached status glyph and the tooltip info-tip now take the chevron's place, as in Selector and MultiSelector. Nothing blocks.
Non-blocking:
- Miss (non-blocking): with
statusVariant="tooltip"and a status that has no message,useInputStatusIconreturns the plain glyph, butComplexSelector.tsx:930still wraps it to stop the press, so a press at the trigger's end opens nothing; wrapping only when the info-tip renders (for exampleisTooltipStatus && status?.message) fixes it.
[Automated review]
Fixes #8956.
User impact
A
ComplexSelectorwith an error, warning or success status, in the defaultattachedplacement, now shows the status icon on its trigger in place of the chevron, asSelectorandMultiSelectordo.tooltipplacement's lost message (point 1 of the issue). The trigger row already shows a focusable info-tip that holds the message and describes the trigger. This PR records that and adds a test reference.Spec
component:ComplexSelectorFR7 adds that with attached status the trigger row shows the status icon in place of the chevron, and with tooltip status it shows, in place of the chevron, a focusable status button that holds the message and describes the trigger. The owner ruled that the glyph replaces the chevron, as in Selector and MultiSelector. The verification row names both tests.Writing rubric results
ComplexSelector.spec.md: FR7 (one sentence added) and its verification row:Changes
ComplexSelector.tsx:useInputStatusIconnow gets the status for every placement. The hook renders a plain icon forattached, nothing fordetached, and the info-tip fortooltip.detachedkeeps the chevron.[fix]patch, credited to @duytbp, who reported it.Evidence
New and changed tests:
Suites: the ComplexSelector suites pass (68).
Checks: strict lint, tsc and
check:knowledgeare clean.Real Chrome (Chromium 154): in the ComplexSelector "Audit matrix" story, against Storybooks Vercel built from main and from this head, at 1280 px and 390 px:
The "Custom trigger selector" row is a caller-rendered control (
renderTrigger) with its own icon, so ComplexSelector's chrome isn't involved and it isn't counted.