Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsPopover (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
This pull request is waiting on a spec decision: anchorRect adds a way to anchor Popover that the current component:Popover contract does not cover, and DEC-5, FR9 and the AR1/AR4 clauses are still pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. The decision should also settle whether attaching usePopover's triggerRef to a caller's own element already covers this need. Whatever the decision, three fixes are needed:
- The documented example in
Popover.doc.mjspasses nullable state thatanchorRect's type rejects, and withundefineda closed Popover falls back to trigger mode and warns aboutchildren; show the conditional mount the story uses, or acceptnull. - FR6 still says the anatomy contains a caller trigger; amend it together with DEC-5, and define where focus goes in AR4 when the element focused before opening is gone.
- The transform correction only undoes translation, so inside a
scale()ancestor the popover overlaps the row; place the anchor box outside such ancestors, or limit the FR9 and DEC-5 wording to translated ancestors.
[Automated review]
| { | ||
| label: 'Opened by the app beside a list row', | ||
| code: `<Popover | ||
| anchorRect={rowRect} |
There was a problem hiding this comment.
This example sets the state to null (setRowRect(null)), which anchorRect?: PopoverAnchorRect rejects (TS2322: Type 'PopoverAnchorRect | null' is not assignable to type 'PopoverAnchorRect | undefined'). Written as rowRect ?? undefined instead, the closed Popover falls back to wrapped-trigger mode: it renders an empty anchor wrapper and warns that children must contain a button. Showing the story's conditional mount here ({rect != null && <Popover anchorRect={rect} isOpen … />}), or accepting null as the closed state of rectangle mode, fixes both problems.
| { | ||
| name: 'Trigger element', | ||
| required: true, | ||
| required: false, |
There was a problem hiding this comment.
FR6 in Popover.spec.md still says the anatomy "contains the caller trigger and content". Making the trigger optional here needs FR6 amended together with DEC-5.
| const dx = rectLeft - placed.left; | ||
| const dy = rectTop - placed.top; | ||
| if (Math.abs(dx) >= 0.5 || Math.abs(dy) >= 0.5) { | ||
| box.style.setProperty('translate', `${dx}px ${dy}px`); |
There was a problem hiding this comment.
This applies a viewport-space difference as a local translate, which is exact only for translation. In a column under transform: scale(0.75), the box renders at 75% of the row's size and the popover starts 23 px inside the row's end. Rendering the box outside any ancestor that can become its containing block (for example, a portal to document.body) would remove the need for the correction. Otherwise, FR9 and DEC-5 should say translated ancestors only.
| - **AR4 — Focus return.** Dismissal returns focus to the trigger when focus had | ||
| entered the closing Popover and would otherwise be lost. | ||
| entered the closing Popover and would otherwise be lost; on an anchor | ||
| rectangle, to the element focused before it opened. |
There was a problem hiding this comment.
This clause doesn't say where focus goes if the element focused before opening has unmounted, for example a menu item whose menu closed. useFocusTrap skips an element that is no longer connected, so focus falls to the page body.
c44c964 to
c961b1d
Compare
There was a problem hiding this comment.
This pull request is waiting on a spec decision: anchorRect adds an anchor source that the current component:Popover contract does not cover, and DEC-5, FR9 and the AR1/AR4 clauses are still pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and the decision should also settle whether attaching usePopover's triggerRef to a caller's own element already covers this need. Whatever the decision, three fixes are needed:
- Under a
scale()ancestor the popover covers the end of the row, because the offset inPopover.tsxonly undoes translation; render the anchor box outside transformed ancestors, or limit FR9 and DEC-5 to translated ones. - The documented example in
Popover.doc.mjspasses nullable state thatanchorRectrejects (TS2322), andundefinedfalls back to trigger mode with achildrenwarning; show the conditional mount the story uses, or acceptnull. - FR6 still says the anatomy contains a caller trigger; amend it together with DEC-5.
[Automated review]
…wport rectangle A popover the app opens beside a list row, a link that keeps its own semantics, a text selection or a point had no anchor: anchorRef must reference a button and wires trigger state and handlers onto it, and an anchorRef without one is not anchored at all. anchorRect takes a rectangle in viewport coordinates, the shape getBoundingClientRect() returns; Popover places an invisible box there (never painted or hit, hidden by opacity because a visibility-hidden anchor hides its layer, offset after layout so a transformed ancestor does not move it) and anchors the layer on it. It moves when a new rectangle is passed, no element gets trigger ARIA, and focus returns on close to the element that had it. Beside children or anchorRef the rectangle wins with a development warning, and an uncontrolled one warns that nothing can open it. Proposed FR9 and DEC-5 of component:Popover record it, with a concept row, AR1 and AR4 notes and a verification row. An AnchorRect story and a Chromium spec check the placement, the box taking no pointer, focus return on Escape and a transformed ancestor; jsdom cases check the box, the absence of trigger state, a moved rectangle and both warnings.
…ntent once, in the sheet Main's Popover now presents as a BottomSheet when presentation resolves to bottom-sheet (DEC-6), rendering the anchored layer and the sheet as two surfaces. On the rebase, rectangle mode renders its invisible anchor box and then those same two surfaces instead of its own popover.render call, so a sheet presentation opens the sheet and holds the content there once, and the rectangle anchors nothing. FR10 (renumbered from FR9, which main gave to the finger growth rule) says so. A new PopoverPresentation case opens an anchorRect popover with presentation="bottom-sheet": the sheet is open and holds the one Apply button, and no anchored layer is open; it fails with rectangle mode's previous direct popover.render.
c961b1d to
23a839b
Compare
There was a problem hiding this comment.
This pull request is waiting on a spec decision: anchorRect adds an anchor source that the current component:Popover contract does not cover, and DEC-5, FR10, the Anchor rectangle concept row and the AR1/AR4 clauses are still pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and the decision should also settle whether attaching usePopover's triggerRef to a caller's own element already covers this need. Whatever the decision, three fixes are needed:
- Under a
scale()ancestor the popover covers the end of the row, because the offset inPopover.tsxonly undoes translation; render the anchor box outside transformed ancestors, or limit FR10 and DEC-5 to translated ones. - The documented example in
Popover.doc.mjspasses nullable state thatanchorRectrejects (TS2322), andundefinedfalls back to trigger mode with achildrenwarning; show the conditional mount the story uses, or acceptnull. - FR6 still says the anatomy contains a caller trigger; amend it together with DEC-5.
[Automated review]
Main's meta#8608 (DEC-9, a Popover is a focus region) added two Tab rows to the focus-entry matrix beside this branch's anchor-rectangle row; both stay.
There was a problem hiding this comment.
This pull request is waiting on a spec decision: anchorRect adds an anchor source that the current component:Popover contract does not cover, and DEC-5, FR10, the Anchor rectangle concept row and the AR1/AR4 clauses are still pending owner review. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and the decision should also settle whether attaching usePopover's triggerRef to a caller's own element already covers this need. Whatever the decision, three fixes are needed:
- Under a
scale()ancestor the popover covers the end of the row, because the offset inPopover.tsxonly undoes translation; render the anchor box outside transformed ancestors, or limit FR10 and DEC-5 to translated ones. - The documented example in
Popover.doc.mjspasses nullable state thatanchorRectrejects (TS2322), andundefinedfalls back to trigger mode with achildrenwarning; show the conditional mount the story uses, or acceptnull. - FR6 still says the anatomy contains a caller trigger; amend it together with DEC-5.
[Automated review]
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
|
I'm pausing this work for now to reduce contention on the repo. Thank you for the reviews and the spec guidance. |
User need
What led to it: this comes from moving every menu and picker in our app onto Astryx, which began with a press, slide and lift bug in its phone menus. In our app, a list row's menu opens a "Group with…" picker beside the row, where there is no button to anchor on.
Why:
Popoveranchors only on a trigger, so an app draws an invisiblerole="button"box in the right place, which announces a fake button and breaks if that box is hidden.Problem and solution fit
PopovertakesanchorRect, agetBoundingClientRect()rectangle, and sits beside it as beside a trigger, following a new rectangle when the caller passes one. No trigger ARIA is added anywhere, and focus returns where it was. It warns besidechildrenoranchorRef, or withoutisOpen. Following scroll is out of scope.Current specification
component:Popover(current) anchors only on a trigger (AR1). This PR proposes a concept row, FR10 and DEC-5, pending owner review; under the sheet presentation the sheet holds the content.Public delta and caller burden
anchorRectand typePopoverAnchorRect; without it nothing changes. The caller opens it withisOpenand passes a new rectangle when its target moves.Design and alternatives
anchorRefon any element. Simple, but often there is no element: a text selection, a point in a canvas.Popoverturns into an invisible anchor box (chosen). Any caller can measure one, and the box is never painted, hit or announced. The cost: the caller updates the rectangle when its target moves.Evidence
Four cases in
Popover.test.tsxfail onmain: the hidden anchor, no trigger ARIA, a new rectangle moving the popover, and the warnings. A Chromium test checks placement beside the row, that the row still takes clicks, focus return on Escape, and a transformed ancestor. On this head every CI check passes exceptpr-rtl, which runs the native touch-target auditmainadded in #7848 after this branch last tookmain; it clears when the branch next mergesmain.Writing rubric results
R1 narration: none; present tense.
R2 research: none.
R3 evidence: FR10 cites DEC-5 and the tests.
R4 rejected alternatives: two
Rejected:lines in DEC-5.R5 change references: none.
R6 open questions: none.
Scope
Testing
In Storybook, open Core/Popover → Anchor Rect.