Repository navigation
feat(core): InfoTip moves to Core as the info preset over Tooltip's press toggle - #9238
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| InfoTip · Storybook | 37.8 kB (new) · 6.2 kB CSS (new) | 113 (new) | — · ✓ · ✗ |
| lab/InfoTip · Storybook | 🔴 87.0 kB (+60.9 kB) · 20 B CSS (−438 B) | 113 (new) | — · — · — |
| charts/Chart | 62.8 kB (0) · 20 B CSS (0) | — | — · — · — |
1 more component
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| charts/ChartTooltip | 52.3 kB (0) · 20 B CSS (0) | — | — · — · — |
Package size:
- @astryxdesign/cli 3.31 MB packed (−8 B); no new dependencies
- @astryxdesign/core 5.96 MB packed (+6.3 kB); no new dependencies
API changes: core +6 · ~0 · −0
6 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Components · component InfoTip |
component InfoTip(props: InfoTipProps) |
<InfoTip /> |
| + | core |
. · Types · interface InfoTipProps |
interface InfoTipProps extends Pick<BaseProps, 'xstyle' | 'className' | 'style'> (props of InfoTip) |
import type {InfoTipProps} from '@astryxdesign/core' |
| + | core |
. · Types · type InfoTipSize |
type InfoTipSize = IconSize |
import type {InfoTipSize} from '@astryxdesign/core' |
| + | core |
./InfoTip · Also exported from an earlier entry point · InfoTip |
InfoTip (.) |
import {InfoTip} from '@astryxdesign/core/InfoTip' |
| + | core |
./InfoTip · Also exported from an earlier entry point · InfoTipProps |
InfoTipProps (.) |
import {InfoTipProps} from '@astryxdesign/core/InfoTip' |
| + | core |
./InfoTip · Also exported from an earlier entry point · InfoTipSize |
InfoTipSize (.) |
import {InfoTipSize} from '@astryxdesign/core/InfoTip' |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Current run: CI concluded failure; parts of this report may be missing.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the InfoTip promotion follows component:InfoTip, but the stacked pressTrigger work has defects to fix first, and the branch should be split.
- The docsite build fails: InfoTip's ShadCN route moves from
components/lab/info-tipwith no alias, so addregistry: {aliases: ['lab/info-tip']}toInfoTip.doc.mjs(as Drawer does) and refresh the route lock. - Land #9232 first and rebase, so this pull request carries only the InfoTip commit (about 1,600 of its 3,900 authored lines).
- A labelled HoverCard whose trigger is not a button loses
aria-controls/aria-expandedunderpressTrigger="toggle"(HoverCard.tsx:380), though FR9 keeps it ontapbehavior. - The pin announcement and the outside-press dismissal in
usePressPin.tsduplicateuseAnnounceRenderedText(which leaves outaria-hiddentext) anduseTouchTrigger's outside dismissal; reuse those owners. - DEP-0016 needs its
@deprecatedmetadata ontap, and FieldLabel and the Tooltip story still usetouchTrigger="tap".
[Automated review]
|
|
||
| // Under the toggle, the press pin owns aria-expanded (true only while | ||
| // pinned) and aria-controls (FR10); the popup role stays here. | ||
| const ownsToggleState = pressTrigger !== 'toggle'; |
There was a problem hiding this comment.
This hands aria-expanded and aria-controls to the press pin for every toggle trigger, but usePressPin wires only buttons. A labelled HoverCard on a link or span still opens on tap and now has neither attribute, unlike under touchTrigger="tap" (HoverCard FR3, FR9). Could this follow isPressPinTrigger(firstChild) instead of the prop?
| const isExpanded = (isOpen !== undefined ? isOpen : isPinned) && isLayerOpen; | ||
|
|
||
| // While pinned, a press or focus outside the trigger and the layer closes it. | ||
| useEffect(() => { |
There was a problem hiding this comment.
useTooltip and useHoverCard already call useTouchTrigger, whose armOutsideDismiss owns the press-outside close and arms from the opening handler. This second listener closes on pointerdown where that one closes on lift. Extending that owner and arming it from toggle() would keep one path.
| // Said once when the layer pins; a preview says nothing beyond the | ||
| // trigger's description. | ||
| const announcedRef = useRef(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
The callers feed this raw textContent, so aria-hidden text is read and blocks run together ("Ada Lovelace★Analyst"). spec:AST-086 FR12 and react-component-runtime INV10 route own-content announcements through useAnnounceRenderedText, with no Effect of the adopter's own.
| const mergedRef = useMergedRefs(ref, buttonRef); | ||
|
|
||
| // Under a coarse pointer the button takes taps out toward 44 px (AR1). | ||
| useEffect(() => { |
There was a problem hiding this comment.
The watch starts because the button attached. A callback ref merged with ref (React 19 ref cleanup) can start and stop it, and it handles node replacement, so this Effect can go.
| | downstream | In the deprecating release, Core FieldLabel's info icon moves from `touchTrigger="tap"` to `component:InfoTip` with `pressTrigger="toggle"` (`component:FieldLabel` FR9), and Lab InfoTip and the CLI page templates migrate too, the templates to a button trigger with `pressTrigger="toggle"` so they carry no development warning (`spec:AST-017` FR29c); docs, stories, templates, and examples teach only `pressTrigger` | | ||
| | transition | no prior transition | | ||
| | state | `proposed` | | ||
| | state | `deprecated` | |
There was a problem hiding this comment.
deprecated here comes before DEP-0016's downstream is done: FieldLabel.tsx:316 still passes touchTrigger="tap", and Tooltip.stories.tsx:373 still teaches it (spec:AST-017 FR29b, FR29c).
|
Fixed in 0812a94: the hit-area watch now starts from a callback ref merged with |
There was a problem hiding this comment.
The InfoTip promotion follows component:InfoTip, but approval waits on an engineering owner's decision about two new @astryxdesign/core/Layer exports and on a few fixes in the stacked pressTrigger commit.
LayerPressTriggerandarmOutsideDismissonuseTouchTrigger's return become public API that no current record decides; an engineering owner records whether they stay public by commenting/approve-spec <head SHA>(the author may, if they are one), or they can be kept internal.- The pin announcement reads raw
textContentin an Effect of its own, soaria-hiddentext is spoken and paragraphs run together ("Revenue30-day rolling average."); announce throughuseAnnounceRenderedTextinstead (spec:AST-086FR12). - The
touchDismissRefEffects inuseTooltipanduseHoverCardonly copy callbacks into a ref; assign the ref during render and drop them. - Switching
pressTriggerfromnonetotoggleafter mount never wires the pin; re-attach when the prop changes and add a rerender test. - Land #9232 with these fixes, then rebase so this pull request carries only the InfoTip commit (about 1,600 of its 3,400 authored lines).
[Automated review]
| return; | ||
| } | ||
| announcedRef.current = true; | ||
| const text = optionsRef.current.announcement()?.trim(); |
There was a problem hiding this comment.
announcement() hands this raw textContent, so aria-hidden glyphs are spoken and blocks run together (an InfoTip with two paragraphs is announced "Revenue30-day rolling average."). spec:AST-086 FR12 and react-component-runtime INV10 route own-content announcements through useAnnounceRenderedText, with no Effect of the adopter's own.
| useEffect(() => { | ||
| touchDismissRef.current = {arm: armOutsideDismiss, disarm: clearTapOpen}; | ||
| }, [armOutsideDismiss, clearTapOpen]); |
There was a problem hiding this comment.
This Effect synchronizes no external system (react-component-runtime INV3); it only copies two callbacks into a ref. Assigning touchDismissRef.current during render, as useTouchTrigger does for hideRef, removes it; useHoverCard.tsx:450 has the same one.
| } | ||
|
|
||
| triggerRef.current = el; | ||
| attachPressPin(el); |
There was a problem hiding this comment.
The pin attaches only when this ref re-attaches, and the dependencies leave out pressTrigger, so a Tooltip rerendered from none to toggle never wires it: no aria-expanded, and a click does nothing. Could attach re-run when pressTrigger changes?
There was a problem hiding this comment.
The InfoTip promotion follows component:InfoTip, but approval waits on an engineering owner's decision about the anchorRef exception this branch writes into component:Tooltip FR9, which today says a pinning press does not reach a clickable row, card, or container around its trigger. An engineering owner can approve that exception by commenting /approve-spec 16655a6e24dbd7f67bb74d88ea6866437c463431 on this pull request (the author may, as an engineering owner), or anchored and useTooltip triggers can stop the press too so FR9 stays as written. Either way, please land #9232 first and rebase, so this pull request carries only the InfoTip commit (about 1,600 of its 3,600 authored lines) and resolves the conflicts with main in Tooltip.spec.md, useTooltip.tsx and useTooltip.doc.mjs.
[Automated review]
| | FR6 | Escape closes a visible tooltip; Escape during text composition is ignored. Escape routing follows `family:overlay-dismissal`: a visible topmost tooltip in the page handles that Escape, so a layer underneath stays open for that press. Implementation gap: a tooltip rendered into another document, such as an iframe or a Picture-in-Picture window, with no layer beneath it is never closed by Escape (Current-state impact). `spec:AST-003` FR4, accepted and not yet implemented, makes an unpinned tooltip passive and gives a pinned one Escape first (Current-state impact). Outside `triggerBehavior="toggle"`, pressing an uncontrolled tooltip's trigger closes it; a press that focuses a text-entry trigger reopens it through FR4. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the tooltip shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled tooltip answers Escape by calling `onOpenChange(false)` without hiding, and is not closed by pressing the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, any focus FR4 opens on, keyboard focus or a click into a text-field trigger, still opens the tooltip and reports `onOpenChange(true)`, and Escape then only reports `onOpenChange(false)`, so it stays until focus or the pointer leaves the trigger or the tooltip, or the caller re-renders with a new `onOpenChange` (Current-state impact). `isEnabled={false}` disables triggers and drops a pending open. | Current source and tests | | ||
| | FR8 | `onOpenChange` reports each show and hide. `hasHoverIndication` (`auto`: a single string or number child) adds a hover cue to the inline trigger. | Current source and tests | | ||
| | FR9 | With `triggerBehavior="toggle"` and a trigger whose role is `button` (a `<button>` or `role="button"`), a click, a completed tap, Enter, or Space on the trigger pins the tooltip open, and the same press on a pinned tooltip closes it. Hover and keyboard focus still preview it per FR3 and FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned tooltip stays open when the pointer leaves the trigger and the surface; Escape (FR6), a second press, a press outside the trigger and the surface, or focus moving outside them closes it. In a controlled tooltip a press reports the toggled value through `onOpenChange` and does not change the state itself (FR7). A press that pins or closes the layer stops at its trigger: it does not reach a clickable row, card, or container around it, or the control a label names. With `anchorRef`, or `useTooltip` with a caller's trigger, the caller's own handlers still receive the press: the anchor's `onClick` runs, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it; Astryx clickable rows, cards, and containers ignore it. A trigger whose role is not `button` gets a development warning and keeps tap-only opening: every completed tap opens the tooltip, and no press pins it. A trigger whose tooltip repeats its own visible text, such as a truncated Text that is its own tab stop, may take `triggerBehavior="toggle"` without being a button: a press pins it as on a button trigger, and it gets no development warning, no `aria-expanded` or `aria-controls`, and no announcement, since its text is already its content. Implementation gap: the own-text clause is not built in Core yet (Current-state impact). | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The anchorRef sentence added to FR9 lets a pinning press reach a plain onClick element around the anchor, while the current FR9 says the press "does not reach a clickable row, card, or container around it". An engineering owner needs to approve this exception (or anchored triggers need to stop the press too) before it can be relied on.
There was a problem hiding this comment.
The InfoTip promotion itself follows component:InfoTip, but the branch needs a rebase before it can land.
- The branch still carries the commit from #9232, which has already merged, so this pull request shows about 3,600 authored lines and conflicts with
maininusePressPin.ts,usePressPin.test.tsx,TouchTrigger.a11y.chromium.spec.ts,TooltipToggleBehaviorA11y.stories.tsxandTooltip.spec.md; please rebase ontomain, drop that commit, and keepmain's versions of those files from #9406.
After the rebase the InfoTip commit applies with no conflicts at about 1,600 lines, and its press pin, catalog name, Lab re-export and 44 px touch target all check out against the record. One non-blocking wording suggestion for a comment is inline.
[Automated review]
| // The tooltip is the button's only purpose. A click, a tap, Enter, or | ||
| // Space pins it open, and the press stops at the button. So an InfoTip's | ||
| // content is reachable on a phone and from the keyboard alike. |
There was a problem hiding this comment.
Non-blocking: the middle sentence restates what triggerBehavior="toggle" does. Keeping only the reason reads shorter:
// The help is the button's only purpose. Pinning makes it reachable by touch and keyboard.There was a problem hiding this comment.
Agreed, and noted for the next change to InfoTip.tsx. This push only rebases the commit onto main (#9232 merged), so it keeps the text that was reviewed.
There was a problem hiding this comment.
This looks ready to approve: after the rebase the pull request is the InfoTip commit alone, which applies cleanly to main and matches component:InfoTip FR1–FR4 and AR1. Tests and browser checks confirm the press pin with aria-expanded, the translated default name, the deprecated Lab re-export, and the 44 px touch target that stops halfway to a neighbour.
- The
InfoTip.spec.mdedits only close its implementation and evidence gaps, and the repository's spec gate still needs an engineering owner's/approve-specfor this head.
The earlier wording suggestion for the comment in InfoTip.tsx stays non-blocking.
[Automated review]
|
/approve-spec 1e58eb1 |
github-actions
Bot
removed
the
needs:spec-owner-review
cixzhang
added
the
ci:run
…s press toggle InfoTip moves to Core: a named info-icon button wrapped in Tooltip with pressTrigger="toggle", so hover and keyboard focus preview the help and a click, tap, Enter, or Space pins it open. Its default name comes from the translation catalog, it forwards a ref to the button and takes xstyle, className, and style, and under a coarse pointer it takes taps out toward 44 px through the shared hit area. The Lab export becomes a deprecated re-export of the Core component.
|
/approve-spec 7d2c696 |
There was a problem hiding this comment.
This looks ready to approve: the pull request moves InfoTip to Core as Tooltip's press toggle on a named button, which matches component:InfoTip FR1–FR4 and AR1. Unit tests and Chromium checks confirm the press pin with aria-expanded, the translated default name, the deprecated Lab re-export, and the 44 px touch target that stops halfway to a neighbour.
The earlier wording suggestion for the comment at InfoTip.tsx:162-164 stays non-blocking.
[Automated review]
User need
Touch, keyboard, and screen-reader users can open the help behind an "i" but not keep it open to read it. Lab's InfoTip pins nothing, and its button is named by an untranslated "More information".
Problem and solution fit
Part of matching native iOS touch behavior: #9175 decided that InfoTip is Tooltip with
triggerBehavior="toggle"on a named button (DEC-1).Current specification
component:InfoTip(#9175, renamed by #9379) andcomponent:TooltipFR9–FR10. Its record edits close the gaps and link Lab's re-export by path; they need an engineering owner's/approve-spec.Public delta and caller burden
InfoTip:content,label,size, arefto the button,xstyle,className, andstyle.InfoTipbecomes a deprecated re-export for one release, and the ShadCN registry keeps its Lab route as an alias.Design and alternatives
Evidence
aria-expanded,aria-controls), and pointer leave keeps it; ref and style reach the button; the name comes from the catalog.Scope
Writing rubric results
InfoTip.spec.md: R1, R2, R4, R5, R6: none. R3: each verification cell names its test.Testing
pnpm test:uiandtest:node, the a11y contract, and the InfoTip and Tooltip browser tests (also 2-core pinned, and WebKit) pass. The press, ref, catalog,toggle, and hit-area cases fail without their change.