Repository navigation
feat(Tokenizer): presentation is a stable prop, and adaptive is its default - #9796
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 2bab017 |
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.
Changes requested. The stable presentation prop, its adaptive default, the alias, the codemod and the composite pins match the approved Tokenizer record. Two things block merging:
packages/core/src/Field/__tests__/SearchKeyboard.interaction.browser.spec.ts(lines 42-47) still expects a combobox on the default Tokenizer story, which the phone interaction lane now renders as the sheet trigger, sopr-interactionwill fail once the pull request leaves draft; point that case at a story pinned topresentation="popover"and add a sheet case that taps the "Team Members" trigger and checks the sheet's search field.- The
[breaking]Changeset cannot merge whilemaindeclares 0.7.7 (pnpm check:changesets, AST-017 FR31); merge it aftermaindeclares 0.8.0, or land the patch-safe half described in the pull request first.
Optional: the comment at Tokenizer.tsx:796-798 could read "An open sheet keeps its presentation until it closes. A resolution change during a task does not replace the open surface."
[Automated review]
| // open sheet keeps its presentation until it closes, so a resolution change | ||
| // mid-task cannot pull the surface out from under the person. | ||
| const presentation = | ||
| presentationProp ?? experimental?.presentation ?? 'adaptive'; |
There was a problem hiding this comment.
With this default, core-tokenizer--default renders the sheet trigger in the phone interaction lane (402×714, coarse pointer), so the unchanged Tokenizer case in packages/core/src/Field/__tests__/SearchKeyboard.interaction.browser.spec.ts (lines 42-47) no longer finds a combobox. Point that case at a story pinned to presentation="popover" and add a case that opens the sheet and checks its search field.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': minor | |||
There was a problem hiding this comment.
pnpm check:changesets refuses this minor [breaking] entry while main declares 0.7.7 (AST-017 FR31). It can merge once main declares 0.8.0; until then the patch-safe half can land on its own.
| // `adaptive` resolves through the policy the selection controls share; an | ||
| // open sheet keeps its presentation until it closes, so a resolution change | ||
| // mid-task cannot pull the surface out from under the person. |
There was a problem hiding this comment.
Non-blocking: this is one 32-word sentence with three topics and an idiom. Suggested:
// An open sheet keeps its presentation until it closes. A resolution change
// during a task does not replace the open surface.
2bab017 to
d53419d
Compare
|
/approve-spec d53419d |
There was a problem hiding this comment.
Changes requested. The stable presentation prop, its adaptive default, the deprecated alias, the codemod and the composite pins match the Tokenizer record approved at this head, and the earlier phone-lane test and comment findings are fixed. Two things block merging:
- The
[breaking]Changeset (.changeset/tokenizer-presentation-default-adaptive.md) cannot merge whilemaindeclares 0.7.7 (pnpm check:changesets, AST-017 FR31); merge it aftermaindeclares 0.8.0, or land the patch-safe half described in the pull request first. - Line 63 of
docs/specs/components/Tokenizer/Tokenizer.spec.mdhas unescaped|inside'popover' | 'bottom-sheet' | 'adaptive', so GitHub renders the Public contract row as "A stablepresentationprop,'popover'" and drops the rest; escape both pipes as|` and restore the spaces around the code spans.
[Automated review]
| | Governing rules | This canonical `component:Tokenizer` record: [FR13–FR27](#behavioral-and-layout-contract), [AR5–AR6](#accessibility-contract), [DEC-2](#dec-2--focus-bootstrap-supports-consecutive-committed-selections), [DEC-3](#dec-3--the-touch-sheet-presentation-is-an-experimental-option), and [DEC-4](#dec-4--the-sheet-edits-a-draft-that-save-commits). | | ||
| | Area | Contract | | ||
| | ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Public contract | A stable `presentation` prop, `'popover' | 'bottom-sheet' | 'adaptive'`with`adaptive`the default, selects the surface (DEC-3); the sheet edits a draft committed by Save (DEC-4).`TokenizerProps`stays an interface; its`ref`is`Ref<HTMLInputElement \| HTMLButtonElement>`because it reaches the sheet's trigger.`experimental.presentation`stays as a deprecated experimental alias. The existing`hasEntriesOnFocus` mode has a defined post-selection lifecycle (DEC-2). | |
There was a problem hiding this comment.
This code span holds two unescaped |, so GitHub-flavoured Markdown splits the row into four cells and drops the last two: the row renders as "A stable presentation prop, 'popover'" and the rest of the contract disappears. Escape both as |, as Ref<HTMLInputElement | HTMLButtonElement>` already is, and restore the spaces the formatter removed around the code spans:
| Public contract | A stable `presentation` prop, `'popover' \| 'bottom-sheet' \| 'adaptive'` with `adaptive` the default, selects the surface (DEC-3); the sheet edits a draft committed by Save (DEC-4). `TokenizerProps` stays an interface; its `ref` is `Ref<HTMLInputElement \| HTMLButtonElement>` because it reaches the sheet's trigger. `experimental.presentation` stays as a deprecated experimental alias. The existing `hasEntriesOnFocus` mode has a defined post-selection lifecycle (DEC-2). |
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': minor | |||
There was a problem hiding this comment.
pnpm check:changesets refuses this minor [breaking] entry while main declares 0.7.7 (AST-017 FR31). Merge it after main declares 0.8.0, or land the patch-safe half described in the pull request first.
d53419d to
a089127
Compare
|
/approve-spec a089127 |
…efault
Tokenizer's touch sheet presentation graduates from
`experimental.presentation` to a stable top-level `presentation` prop
(`'popover' | 'bottom-sheet' | 'adaptive'`, typed by the new exported
`TokenizerPresentation`), and an omitted `presentation` resolves
`adaptive`: the sheet on a compact coarse-pointer screen, and the inline
combobox with a fine pointer, on a screen that is not compact, on the
server, and while hydrating.
`experimental.presentation` stays as a deprecated experimental alias; the
top-level prop wins when both are set, with one development warning. A
staged codemod rewrites a static `experimental={{presentation: x}}` to
`presentation={x}`.
Core composites that render Tokenizer as an inline field pin
`presentation="popover"`: PowerSearch's field and its list value editors,
and Table's list filter. Stories, blocks, templates, and the sandbox page
that pass a prop the sheet does not render pin it too.
The default change is `[breaking]` under spec:AST-017 FR1, FR3, and FR31,
so it is held for a declared minor; the promotion is `[feat]`.
a089127 to
2bd3a08
Compare
|
/approve-spec 2bd3a08 |
There was a problem hiding this comment.
Changes requested. The stable presentation prop, the deprecated alias, the codemod and the composite pins match the Tokenizer record approved at this head, and the earlier test, comment and record-formatting findings are fixed. One thing blocks merging:
- The
adaptivedefault changes a released functional contract on phones (onChangetiming, therefelement and the inline-only props), sospec:AST-017FR1 and FR3 classify it as breaking, and FR6, FR31 and FR54 allow it only as a[breaking]Changeset in a declared minor, not as the patch[feat]in.changeset/tokenizer-presentation-stable.mdwhilemaindeclares 0.7.7.
To fix it, restore the [breaking] entry and merge once main declares 0.8.0, or land the stable prop with a popover default as a patch now and flip the default in the minor; either way, drop the patch-tier sentences from the Tokenizer record and the Changeset. If the owners want this default in a patch, spec:AST-017 has to change on main first.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Tokenizer's `presentation` prop is stable, and its default is `'adaptive'`. `presentation` takes `'popover' | 'bottom-sheet' | 'adaptive'`, typed by the new exported `TokenizerPresentation`. The API change is additive: the prop is new and optional, no existing prop, type or export is removed or narrowed (`TokenizerProps['ref']` keeps its input type, and the component also accepts a ref typed `Ref<HTMLInputElement | HTMLButtonElement>`), and `experimental={{presentation: x}}` keeps working as a deprecated experimental alias that `astryx upgrade` rewrites to `presentation={x}`; when both are set, `presentation` wins and the field warns once in development. The behavior changes only on compact coarse-pointer screens (phones), which is what `'adaptive'` is for: there a Tokenizer with no `presentation` shows a trigger that opens the bottom sheet, the sheet edits a draft so `onChange` reports once, on Save, `ref` receives the trigger button, and `renderItem`, `renderToken`, `endContent`, `hasClear`, `hasEntriesOnFocus`, `menuWidth`, `tokenOverflowBehavior` and `hasAutoFocus` are not rendered, with one development warning that names them. With a mouse or trackpad, on a screen that is not compact, in the server render and while hydrating, nothing changes. Pass `presentation="popover"` to keep the previous behavior on every device. Shipping this default in a patch is a product decision of the engineering owner, agreed with the owner of the compatibility policy. |
There was a problem hiding this comment.
This entry ships the adaptive default as a patch [feat]. On a phone, a released caller that uses renderToken, endContent or hasClear, saves on each onChange, or calls input methods on ref must add presentation="popover" to keep working. spec:AST-017 FR1 and FR3 make that a breaking change. FR6 and FR54 say an owner decision cannot make it patch-eligible, and FR31 refuses [breaking] while main declares 0.7.7. Restore the [breaking] entry and merge once main declares 0.8.0. Or ship the prop with a popover default now and flip the default in the minor.
| * stays the component's declared props type: it is an interface, so | ||
| * `interface X extends TokenizerProps<T>`, `Omit<TokenizerProps<T>, …>` and | ||
| * `ComponentProps<typeof Tokenizer>` keep their released shapes. | ||
| * The props the component accepts: `TokenizerProps`, whose `ref` keeps its |
There was a problem hiding this comment.
Non-blocking: the first sentence restates the type below it. Keep only the reason:
/**
* `TokenizerProps` stays the declared props type, so `interface X extends
* TokenizerProps<T>`, `Omit<TokenizerProps<T>, …>` and
* `ComponentProps<typeof Tokenizer>` keep their released shapes.
*/
Owner ruling (engineering owner), agreed with the AST-017 owner: the stable presentation prop and its adaptive default ship as a [feat] patch. The API does not change (the stable prop is new and optional; the experimental spelling stays as a deprecated alias); the behavior change is on compact touch screens only, by design of adaptive, with desktop and server rendering unchanged; presentation="popover" and the codemod keep the previous behavior. Every other review item is resolved at 2bd3a08.
User need
On a phone, Tokenizer's inline combobox competes with the software keyboard: the suggestion menu opens under the keyboard, and the field moves as tokens wrap. The touch sheet already exists behind
experimental.presentation. The owner asked to make it a stablepresentationprop and to makeadaptivethe default. A phone then gets the sheet without each caller opting in. A mouse, a trackpad, a screen that is not compact, the server render and hydration all keep today's inline combobox.Problem and solution fit
presentation?: 'popover' | 'bottom-sheet' | 'adaptive'is added at the top level, typed by the new exportedTokenizerPresentation. That is the selection controls'AdaptivePresentationvalue set. The default is'adaptive'.experimental.presentationstays as a deprecated experimental alias (@experimentaland@deprecated). When both are set,presentationwins, and the field warns once in development. "Alias decision" below gives the reason.promote-tokenizer-presentationcodemod rewrites a staticexperimental={{presentation: x}}topresentation={x}, also past a spread of props that comes beforeexperimental(the{...args}story pattern). It leaves a TODO, and the working alias, where it cannot prove the rewrite, including a spread afterexperimental. Its own Changeset names@astryxdesign/cli. The repository's own stories are the codemod's output:codemod-verify.mjsreproduces them.reftype.TokenizerProps['ref']keeps its releasedRef<HTMLInputElement>type, so wrappers keep compiling. Under the default,refreaches the sheet's trigger on a phone, so the component also acceptsRef<HTMLInputElement | HTMLButtonElement>. That overload replaces the one that accepted the widerrefonly whenexperimentalwas passed.presentation="popover"where they must not change. The table below lists each one.Tokenizer.doc.mjs(en, zh, dense) documents the stable prop and its default, and labels the alias as deprecated. The stories are renamed without "Experimental:" (Bottom sheet, Adaptive (default), Bottom sheet states). The record graduates DEC-3.api-report.mdis regenerated.Current specification
component:Tokenizer: DEC-3 (rewritten here), FR16, and Compatibility and migration.spec:AST-017: FR3, FR21–FR23, FR29, and DEC-23.architecture:public-component-api: INV16.component:Selector: DEC-1, DEP-0011 and CLN-0020 are the precedent.Release tier: patch, by owner decision
One Changeset,
[feat], patch. The engineering owner ruled the change nonbreaking and ships it in a patch, and the owner ofspec:AST-017agrees. The record (Compatibility and migration, DEC-3) and the Changeset state the reasons:presentationprop is additive: it is new and optional. No existing prop, type or export is removed or narrowed.TokenizerProps['ref']keeps its released type, and the experimental spelling stays as a deprecated alias.'adaptive': compact touch screens get the sheet. Desktop, fine pointers and server rendering are unchanged.presentation="popover"keeps the previous behavior everywhere, and the codemod is available for the alias.spec:AST-017.Precedent. Selector, MultiSelector, DropdownMenu (and MoreMenu), ContextMenu and Popover already ship a stable
presentationwith the same three values.component:SelectorDEC-1 also makesadaptivethe default, withpresentation="popover"as the way back. This PR follows the same shape.Alias decision
experimental.presentationwas not removed. It is in thev0.7.7tag, which was cut today onrelease/v0.7.7, and that release's CHANGELOG lists it. npmlateststill showed 0.7.6 when this PR was written. FR49 makes the newestvX.Y.Ztag the latest stable release, so this experimental surface is released. FR22 would allow an[experimental]removal. FR23 encourages a deprecated alias and asks for a codemod when the rewrite is mechanical, so this PR keeps the alias and adds the codemod. FR29 forbids a runtime warning for plain use, so the only development warning is the precedence warning, shown when both spellings are set.Internal consumers
The rule is simple. A composite whose design is the inline field pins
presentation="popover". An example or template that passes a prop the sheet does not render pins it too, as the development warning asks. Everything else follows the new default.popoverstring_listandentity_listisSheet ? 'bottom-sheet' : 'popover'enum_listpopoverpopoverpopoveruntil CLN-0020.hasEntriesOnFocus)popoverpopover(all three)endContentandhasAutoFocus. The three rows read as one header, so Cc and Bcc match To.renderToken)popoverhasClear,endContent,tokenOverflowBehavior,hasEntriesOnFocus,renderToken), 3 of them throughargs(With Clear All Button, Logical order, Caret placement)popoverhasClear×2)popoverdoc.mjsand the blocksA new test,
Tokenizer.composites.test.tsx, proves the composite pins on a compact coarse-pointermatchMedia: PowerSearch's field, the three list editors and Table's list filter stay inline combobox fields, and PowerSearch's sheet still gets Tokenizer's sheet. When the pins are removed, 5 of its 6 cases fail.Evidence: the phone interaction lane
The interaction lane runs every
*.interaction.browser.spec.tson a 402×714 touch phone, where the default now presents the sheet.SearchKeyboard.interaction.browser.spec.tsopened the default Tokenizer story and expected a combobox. Its inline case now opens the pinned "With end content" story. A new case taps the default story's trigger and checks that the sheet's search field asks for the Search key with no autocorrection. All 7 cases pass in Chromium on this lane locally, and the full Chromium interaction suite passes (512 passed, 7 skipped).Evidence: desktop and server unchanged
Tokenizer.test.tsx,PowerSearch.test.tsxandPowerSearchEditPopover.test.tsxare not edited. Their 185 tests pass on this branch as on main.pointer: fine). The DOM is identical to main for all 26 Tokenizer and 35 PowerSearch stories. Screenshots are identical except forcaret-placement, which differs because of the blinking caret. The same story differs on the PowerSearch branch, which does not touch Tokenizer. Opening the default Tokenizer menu and the default PowerSearch menu gives identical page markup.pointer: coarse, 390px). On main, the default Tokenizer story renders the combobox. On this branch it renders the trigger, and a tap opens the sheet dialog "Team Members". The "With end content" story is pinned, so it stays a combobox. PowerSearch's default story keeps its inner Tokenizer as a combobox.renderToStringoutput is byte-identical to main for four no-prop cases: a Tokenizer that is empty, a Tokenizer with tokens,hasClear,hasAutoFocusanddescription, a PowerSearch that is empty, and a PowerSearch with filters andresultCount.Tokenizer.ssr.test.tsxnow runs the default with no prop. It server-renders the combobox and hydrates with no recoverable error. On a coarse client it swaps to the trigger with focus undisturbed. Its markup equalspresentation="popover"'s.Bundle
The sizes come from the PR size report's measurer (gzip of the eager import).
TokenizerBottomSheetis not in any eager chunk of Tokenizer or PowerSearch.scripts/core-single-bundle.test.mjspasses.Scope
UseLongPressHandlershunk thatapi:reportregenerates on main is left out.Testing
internal/,useAnnounceRenderedTextadopters,slotPresence,touchTriggerTargets, the i18n PowerSearch end-to-end test, the CLI codemods, the CLI golden reads, experimental isolation and the single-bundle test: 172 files and 3,357 tests pass.pnpm typecheckpasses. It runs every workspace typecheck script, as CI does.ASTRYX_STRICT_LINT=1 eslinton the changed files reports 0 errors. Prettier is clean.check:repo. Every step passes,check:changesetsincluded.sync-exports --checkis up to date.@astryxdesign/core,@astryxdesign/cli), andcodemod-verify.mjsreproduces the story edits.check:knowledgeis aligned. The single-bundle test passes.git diff --checkis clean.global-local.test.mjsfails locally onnpm packbecause the local Node install has no npm binary. That is an environment failure; CI runs it.Overlap with open work
transforms/next/index.mjs, a trivial conflict. This PR edits one line inPowerSearch.tsxand two inPowerSearchValueEditor.tsx, and the other PR does not touch those lines.isDateOnly: falsethe time of day it names #9702 (PowerSearch dates). This PR touches neither the PowerSearch record nor the date code.Writing rubric results
docs/specs/components/Tokenizer/Tokenizer.spec.md:popoverdefault only to classify the change, and they state the release tier as the owners' product decision. DEC-3 records that decision with its reasons. FR16 states the present contract. No other narration was added.popoveras the default" was added. Two old lines were removed because this decision overturns them: an automatic switch without an opt-in, and a stable prop before consumer evidence.component:PowerSearch.renderItemandrenderToken. It is unanswered and keeps its options.