Repository navigation
feat(cli): a BaseTypeaheadMenu block - #10625
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.
|
|
/approve-spec 6dd00a9 |
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The BaseTypeaheadMenu block shows the menu config as intended: a heading and a key hint around the list, a named dialog opened from a pick or from a filter button, and focus back on the opener after Escape. One fix remains:
- Picking a field that already has a filter opens its dialog with the field's first value checked instead of the applied one (
BaseTypeaheadMenu.tsx:158), so Apply silently replaces the filter; start from the applied filter, for exampleedit(filters.find(f => f.field.id === field.id) ?? {field, value: field.values[0]}).
CI does not run on this pull request because it targets a feature branch, so CI first covers the block once the stack reaches main.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: the BaseTypeaheadMenu block demonstrates BaseTypeahead's proposed menu contract (FR13–FR16, AR6, DEC-1, and the DEC-2 the latest merge brought in), which is not on main yet, and the earlier /approve-spec covered only the previous head. 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. Two fixes are needed either way:
- Picking a field that already has a filter opens its dialog with the field's first value checked instead of the applied one (
BaseTypeaheadMenu.tsx:158), so Apply silently replaces the filter; start from the applied filter, for exampleedit(filters.find(f => f.field.id === field.id) ?? {field, value: field.values[0]}). - Miss: the block repeats the
Core/BaseTypeaheadMenu story'sMenuExampleline for line (apps/storybook/stories/BaseTypeahead.stories.tsx:332-498), so a fix to one copy misses the other; have the story render the block, asCheckboxList.stories.tsxalready does for its block, and delete the story's copy.
[Automated review]
|
/approve-spec fc80c76 |
There was a problem hiding this comment.
Changes requested. The BaseTypeahead menu direction this block demonstrates (FR13–FR16, AR6, DEC-1 and DEC-2) is now approved for this head, so two fixes remain:
- Picking a field that already has a filter opens its dialog with the field's first value checked instead of the applied one (
BaseTypeaheadMenu.tsx:158), so Apply silently replaces the filter; start from the applied filter, for exampleedit(filters.find(f => f.field.id === field.id) ?? {field, value: field.values[0]}). - Miss: the block repeats the
Core/BaseTypeaheadMenu story'sMenuExampleline for line (apps/storybook/stories/BaseTypeahead.stories.tsx:332-498), so a fix to one copy misses the other; have the story render the block, asCheckboxList.stories.tsxalready does for its block, and delete the story's copy.
Non-blocking: the block row in packages/cli/test/doc-routes.json (line 4428) still counts 669 blocks; bump it to 670.
[Automated review]
|
/approve-spec 7bdfda7 |
There was a problem hiding this comment.
Changes requested. The BaseTypeahead menu direction this block demonstrates is approved for this head, and the earlier fixes are in: a re-pick opens on the applied filter, the Menu story renders the block, and the block count reads 670. One fix remains:
- Rendering the block from the
Core/BaseTypeaheadMenu story drops the field's keyboard focus ring in Storybook (2px on the base, 0px on this head): Storybook's StyleX split (apps/storybook/.storybook/main.ts:200,libraryPattern: 'packages/') puts the block in theastryx-baselayer while it still compiles with the product class prefix, so the story's identicaloutline-width: 0andoutline-color: transparentclasses in the laterproductlayer override the block's:has(> input:focus-visible)ring; classifypackages/cli/assets/templates/as product source in that split, as the docsite already does, and assert the ring inBaseTypeaheadMenu.a11y.chromium.spec.ts.
Non-blocking: BaseTypeaheadMenu.a11y.chromium.spec.ts routes pr-a11y on the story file but not on packages/cli/assets/templates/blocks/components/BaseTypeahead/, so a change to the block alone skips it; add the block's folder to its scope.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision for this head. The BaseTypeaheadMenu block demonstrates BaseTypeahead's menu direction (component:BaseTypeahead FR13–FR16, AR6, DEC-1 and DEC-2), which is not yet in the record on main, and the earlier /approve-spec comments name previous heads only. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
The Storybook focus-ring fix works: with the block compiled under the library class prefix, the Menu story's field draws its 2px ring under keyboard focus again, and the Chromium spec asserts it. No code changes are needed. The block lands together with #10544 and #10624.
[Automated review]
cixzhang
added
the
ci:run-a11y
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision for this head. The BaseTypeaheadMenu block demonstrates BaseTypeahead's menu direction (component:BaseTypeahead FR13–FR16, AR6, DEC-1 and DEC-2), which is not yet in the record on main, and the earlier /approve-spec comments name previous heads only. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
The latest push only merges a Tokenizer fix from the base branch, and the block, the story and the Storybook change are unchanged, so no code changes are needed. The block lands together with #10544 and #10624.
[Automated review]
|
/approve-spec a5be431 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head, because it runs only on pull requests into main that are ready for review.
The BaseTypeaheadMenu block now has an engineering owner's spec approval for this head, and the earlier findings are fixed: a re-picked field opens on its applied value, the Menu story renders the block instead of a copy, the block count reads 670, and the field keeps its keyboard focus ring in Storybook. The block lands together with #10544 and #10624.
[Automated review]
|
/approve-spec 0c34e25 |
There was a problem hiding this comment.
Changes requested: the BaseTypeaheadMenu block, its catalog entries and the story switch look right, and the BaseTypeahead menu direction it demonstrates is approved for this head, but one fix remains.
- Miss:
apps/storybook/.storybook/main.ts:205passeslibraryPatternsinsidestylexOptions, whose type has no such key, sopnpm typecheckinapps/storybookfails with TS2353 and the merge group's typecheck check would fail; move the list into its own constant and spread it intostylexOptions, or type the option in@astryxdesign/build.
The earlier findings (the re-pick of an applied filter, the duplicated story, the block count and the focus ring) stay fixed.
[Automated review]
|
/approve-spec 53cad9d |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
#10544: approve. The BaseTypeahead menu and handleRef records are owner-approved for its head. The earlier size request no longer applies now that the stack targets main. Non-blocking: the changeset sentence at .changeset/typeahead-menu.md:5 ends with a semicolon.
#10624: approve. Tokenizer's FR28 is owner-approved for its head, and the merge of #10544's restore-point fix keeps both focus paths. Non-blocking: FR28's sheet sentence has no test. In Safari, a click on a custom token's control that opens nothing leaves focus on the page.
#10625: approve. The block is unchanged and keeps its earlier fixes. On this head the Menu story shows the listbox notes, the named dialog, focus back on the filter button after Escape, and a 2px field ring.
Whole stack: pass. The API is consistent across the three pull requests, and the stack merges cleanly onto main. A pull request lands only when it and every pull request below it are approved.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| BaseTypeahead | 72.7 kB (0) · 10.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
| Typeahead | 81.5 kB (0) · 12.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TypeaheadItem | 6.4 kB (0) · 3.6 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: @astryxdesign/cli 3.56 MB packed (+1.5 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 36 rows, core 36 rows; run pnpm api:report.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
|
/approve-spec 1f9c787 |
There was a problem hiding this comment.
Changes requested: one lint fix in #10544, then nothing else blocks the stack.
#10544: request changes.
- The new spec waits break the required
lintcheck on all three heads:expect.poll(() => activeOption(page))atpackages/core/src/Typeahead/__tests__/BaseTypeaheadMenu.a11y.chromium.spec.ts:79,:92,:94and:105returns a promise from a callback that is notasync, which CI's strict@typescript-eslint/promise-function-asyncrejects, so make each callbackasync () => activeOption(page)and merge the fix up into #10624 and #10625.
#10624: approve. component:Tokenizer FR28 is owner-approved for this head, and its own increment has no defect.
#10625: approve. The Menu story renders the block with its field ring, the notes around the list, the named dialog and the focus return.
Whole stack: fail on the required lint check; a pull request lands only when it and every pull request below it are approved.
mainhas since changed the same CLI search goldens (#10532), so when you mergemain, regeneratepackages/cli/test/__golden__/no-icon-set/search-icon-set.jsonand.txt.
[Automated review]
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because GitHub reports a merge conflict with main.
#10544: approve. The four spec poll callbacks are now async, so the required lint check passes, and its records are owner-approved for its head.
#10624: approve. component:Tokenizer FR28 is owner-approved for its head, and its own change has no defect.
#10625: approve. The Menu story renders the block with its field ring, the notes around the list, the named dialog and the focus return.
Whole stack: pass. A pull request lands only when it and every pull request below it are approved.
Next, merge main into #10544 and up the stack, and regenerate packages/cli/test/__golden__/no-icon-set/search-icon-set.json and .txt (main changed the same match count in #10532), so CI runs on the new head.
[Automated review]
|
/approve-spec 28d6cbc |
There was a problem hiding this comment.
Changes requested: one scope fix in #10544; nothing else in the stack blocks.
#10544: changes requested.
- Newly live check:
TypeaheadProps.handleRefandTypeaheadProps.menu(Typeahead) are outside this pull request's scope (BaseTypeahead), and no pull request in the stack names Typeahead, so name it in the scope, for examplefeat(BaseTypeahead, Typeahead): …or a## Scopesection withComponents: BaseTypeahead, Typeahead; the approvedcomponent:TypeaheadFR11 already records the direction.
Its size point no longer applies, because #10625 now contains #10624's head and the stack holds.
#10624: changes requested in its own review, both points now met. The stack holds, and the BaseTypeahead and Typeahead rows it listed belong to #10544; a new review of #10624 can clear the request.
#10625: approve. The block, its catalog entries and the Menu story are owner-approved for this head and behave as the records describe.
Whole stack: changes requested until Typeahead is in scope. A pull request lands only when it and every pull request below it are approved.
[Automated review]
|
/approve-spec 2249839 |
There was a problem hiding this comment.
Approved pending CI: the heavy suites (build, test, docsite-test) have not run on this head yet; they run before merge.
#10544: changes requested in its own review, and its one point is now met. The title now names Typeahead, so TypeaheadProps.menu and TypeaheadProps.handleRef are in scope; a new review of #10544 can clear the request.
#10624: approve. component:Tokenizer FR28 is approved for its head, and the forwarding, the handle and the focus return to a token behave as it describes.
#10625: approve. The block, its catalog entries and the Menu story are approved for this head and behave as the BaseTypeahead records describe.
Whole stack: pass. A pull request lands only when it and every pull request below it are approved, so the stack now waits on a fresh approval of #10544. A few optional notes (a Safari focus case in Tokenizer, the editor's first focus on the first radio, and three comments) are in the review.
[Automated review]
|
/approve-spec 2249839 |
Puts the Backspace-to-clear change on top of the menu group (a linear stack).
|
/approve-spec 2249839 |
|
/approve-spec 528ddb7 |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #10847.
528ddb7 to
4daaab9
Compare
|
/approve-spec 4daaab9 |
There was a problem hiding this comment.
Approved pending CI: the last test-ui shard and three pr-interaction shards were still running on this head when this review finished. The BaseTypeaheadMenu block shows only behavior that component:BaseTypeahead already records on main (FR13–FR16, AR6, DEC-2), its catalog entries reach the CLI, docsite, ShadCN registry and sandbox, and the Storybook Menu story renders it with its focus ring intact, so nothing blocks. This pull request was reviewed on its own, because #10544 and #10624 have merged and #10847 does not contain this head yet.
- Non-blocking: the Chromium spec's
@positionsentence no longer lists everything its@pr-a11yscope runs it for (see the inline comment).
[Automated review]
| * and closing the dialog returns focus to the element that opened it. | ||
| * and closing the dialog returns focus to the element that opened it. The | ||
| * field draws its keyboard focus ring. | ||
| * @position Runs in pr-a11y when Typeahead, Tokenizer, Popover or this story |
There was a problem hiding this comment.
Non-blocking: the scope line above now also runs this spec when the BaseTypeahead CLI blocks or Storybook's config change, but this sentence still lists only Typeahead, Tokenizer, Popover and the story. Suggested wording:
* @position Runs in pr-a11y when Typeahead, Tokenizer, Popover, this story,
* the BaseTypeahead CLI blocks or Storybook's config change. jsdom has no
* top layer and no real focus movement on close.
Stack: #10544 → #10624 → #10625 → #10847
This stack lands bottom-first as a chain: #10544 merged; #10624 next; then #10625 and #10847 rebased on each merge, each with a fresh spec stamp, CodeBunny review and CI.
Who this helps
People starting a filter field from the CLI. The CLI gains a
BaseTypeaheadMenublock showing BaseTypeahead'smenu(#10544): a listbox menu with information around the result list, and a pick or filter button that opens a dialog-role menu on the same layer.What changed
packages/cli/assets/templates/blocks/components/BaseTypeahead/BaseTypeaheadMenu.{tsx,doc.mjs}.exampleFor: 'BaseTypeahead'makes it a documented example on BaseTypeahead's own docsite page, in its Examples section next to the existing showcase (/components/BaseTypeahead#example-BaseTypeaheadMenu), as ComplexSelector's examples are.Changeset:
[feat]patch for the CLI.Verification
/components/BaseTypeaheadrenders "BaseTypeahead — Menu" in Examples.experimental-isolation.test.mjs) pass with the new search results (46); the ShadCN route lock regenerated withUPDATE_SHADCN_ROUTE_LOCK=1.