Repository navigation
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
Generated by PR Enrichment workflow | View current CI run |
![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).
…ress `component:Tooltip` and `component:HoverCard` FR9 and FR10, `spec:AST-003` FR4: - `pressTrigger` (`none`, the default, or `toggle`) on Tooltip, HoverCard, useTooltip and useHoverCard, with the types TooltipPressTrigger, HoverCardPressTrigger and LayerPressTrigger. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the layer and the same press closes it; hover and focus still preview it. - A pinned layer stays until Escape, a second press, a press or scroll outside (useTouchTrigger's outside dismissal, which the pin arms), or focus leaving the trigger and the layer (each layer's focus-leave path), and takes Escape before the layer beneath it. - The press stops at its trigger. The trigger carries aria-expanded and aria-controls on a button trigger (a labelled HoverCard keeps its own on any other trigger), and the layer is announced once when it pins, through useAnnounce, so inside a modal too. - A non-button trigger gets a development warning and keeps tap-only opening; a trigger whose layer repeats its own text pins without toggle state (`Layer/usePressPin.ts`).
…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.
|
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?
Stacks on #9232 (
pressTrigger): only the last commit is this pull request's own.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
This is part of making Astryx's touch components behave like native iOS ones. #9175 decided that InfoTip is Tooltip with
pressTrigger="toggle"on a named button (DEC-1).Current specification
component:InfoTip(#9175) andcomponent:TooltipFR9–FR10. Its record edits close the gaps and need the owner's approval.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
docs/specs/components/. Whichever lands second moves this record's edits.Evidence
aria-expanded,aria-controls), and pointer leave keeps it. The ref and style props reach the button, and 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:ui, and the browser test (also 2-core pinned) pass; the node project fails only fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520's cache cases. The press, ref, catalog, and hit-area cases fail without their change.