Repository navigation
fix(MobileTokenizer): refine touch sheet editing flow - #8742
rubyycheung wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
needs:code-review
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMobilePowerSearch (@astryxdesign/lab) · View in Storybook
MobileTokenizer (@astryxdesign/lab) · 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 Regression12 of 40 shot(s) changed. A change here is a question, not a failure: check whether the after is the
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.
Approval waits on an engineering owner's decision, because this PR rewrites the component:Tokenizer touch-sheet contract (FR18, FR19, FR22–FR24 and the DEC-3 counts). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. The owner needs to settle the draft that Save commits (including what change reports), the discard confirmation, the footer staying while searching, the bordered search field, scrim-dismissable confirmations in place of AlertDialog, dismissing the keyboard on list scroll, and BottomSheet restoring the pre-keyboard scroll. Whatever is decided, these need changes:
- After the result list has been scrolled, typing in Search keeps only the first character, because the list's
onScrollblur also fires on the scroll a keystroke causes. - Save reports the last draft action as
change, which can name an item that never entered or left the value, so derive it from the net difference or document the behavior. - The branch conflicts with
main, which now keeps the sheet unexposed: rebase, keepmain's implementation-gap text, drop the consumer-docs edit, make the changeset internal, and credit@rubyycheung. - Dismissing the keyboard on scroll needs agreement in
spec:AST-094andspec:AST-099, plus the iOS Simulator evidencespec:AST-099asks for.
[Automated review]
| focusedSearch != null && | ||
| document.activeElement === focusedSearch | ||
| ) { | ||
| focusedSearch.blur(); |
There was a problem hiding this comment.
This blur also runs for scrolls the person did not make. When a keystroke shrinks a scrolled list, the browser clamps scrollTop and fires scroll, so the field loses focus after the first character: in Chromium, typing "Eng" after scrolling the list leaves "E". Blurring only from the person's own drag (for example touchmove past a small threshold) would avoid it; a test that types after a scrolled list would catch it.
| ? ({item: fallbackRemoved, type: 'remove'} as const) | ||
| : null); | ||
| if (change != null) { | ||
| onCommit([...draftValue], change); |
There was a problem hiding this comment.
change here is the last draft action. Opening with Design and Eng, then checking Energizer, unchecking Design and unchecking Energizer reports onChange([Eng], {item: Energizer, type: 'remove'}), although only Design left the value. Deriving change from the net difference between valueAtOpen and draftValue would keep the docs' "affected item" true.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] Improve Tokenizer's touch sheet with a bordered search field, persistent footer actions, keyboard dismissal while results scroll, draft selections committed by Save, and confirmation before a dirty draft is discarded. |
There was a problem hiding this comment.
main now ships the sheet presentation unexposed, with an internal [chore] changeset. After the rebase, this note should be internal too, not a consumer-facing [experimental] entry.
|
|
||
| [experimental] Improve Tokenizer's touch sheet with a bordered search field, persistent footer actions, keyboard dismissal while results scroll, draft selections committed by Save, and confirmation before a dirty draft is discarded. | ||
|
|
||
| @rubycheung |
There was a problem hiding this comment.
This handle is @rubycheung; your other changeset uses @rubyycheung, and the changelog credits the handle written here.
ccf3642 to
0d1c584
Compare
rubyycheung
changed the title
0d1c584 to
f8bc6c5
Compare
f8bc6c5 to
0e0ac7c
Compare
0e0ac7c to
34da74b
Compare
34da74b to
a40cf71
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the Core BottomSheet change: returning a Tall sheet's body to its pre-keyboard scroll position as the keyboard leaves, including after an iOS dismissal that leaves only the accessory bar, is not stated in the on-screen keyboard spec (spec:AST-099), so an engineering owner needs to settle that behavior and record the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner). Independently of that decision, please fix the points below and rebase onto main, which no longer embeds MobileTokenizer in MobilePowerSearch, so the MobilePowerSearch.test.tsx edits drop out:
- Every
scrollevent on the results list blurs the search, so after the list has scrolled, typing keeps only the first character (MobileTokenizer.tsx:1169–1171). - A keyboard that only gets shorter while a field keeps focus resets a Tall sheet's body to its opening scroll and glides it back (
useMobileKeyboard.ts:338–346). - Re-checking an item selected at open counts as a change, and
changereports only the last draft action, which can name an item that never joined or left the value (MobileTokenizer.tsx:142–146,:1098–1104). - The new layout effect sets state when the sheet closes (
:1038–1046), the changeset credits@rubycheunginstead of@rubyycheung, andspec:AST-099asks for iOS Simulator evidence in a top-level document before keyboard work merges.
[Automated review]
| data-total-count={visibleItems.length} | ||
| onTouchMove={blurSearch} | ||
| onScroll={event => { | ||
| blurSearch(); |
There was a problem hiding this comment.
This blur also runs for scrolls the person did not make. When a keystroke shrinks a scrolled list (the filter, plus the window reset in handleQueryChange), the browser clamps scrollTop and fires scroll, so the search loses focus after the first character: in Chromium, typing "Eng" after scrolling the list leaves "E". Blurring only on the person's own drag of the list (a touchmove past a small threshold after a touchstart on the list) would keep typing intact.
| obstructionTop > keyboardObstructionTop + 0.5 && | ||
| bodyScrollTopAtKeyboard != null | ||
| ) { | ||
| body.scrollTop = bodyScrollTopAtKeyboard; |
There was a problem hiding this comment.
Any growth of the visual viewport lands here, not only an interactive dismissal: a keyboard that gets shorter while the field keeps focus (predictive bar hiding, a keyboard switch, Next to a field with a shorter keyboard) resets the body to its opening scroll, then the safe-area pass smooth-scrolls back. With the field at 660–700px and the keyboard top moving from 500 to 540px, the body goes 248 → 0 → 208. Could the restore be gated on a signal that identifies the dismissal, with a test where the keyboard shrinks and the field stays focused?
| item => !draftValue.some(draftItem => draftItem.id === item.id), | ||
| ); | ||
| const change = | ||
| lastDraftChangeRef.current ?? |
There was a problem hiding this comment.
change here is the last draft action, not the net change. Opening with Design and Eng, then checking Energizer, unchecking Design and unchecking Energizer reports onChange([Eng], {item: Energizer, type: 'remove'}), although only Design left the value; unchecking and re-checking Design reports an add for an item the caller already holds. Deriving change from the difference between valueAtOpen and the draft (and comparing the draft as a set, or restoring a re-checked item's opening position) would keep it truthful.
| shouldRestoreDraftFocusRef.current = true; | ||
| setIsDiscardConfirmationOpen(false); | ||
| }; | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This layout effect sets state after the commit that force-closes the sheet, so that commit still renders an open confirmation. architecture:react-component-runtime INV1 and INV3 apply to packages/lab/src/; closing both confirmations in the existing isOpen !== wasOpen render adjustment, or in the close path, avoids the repair, and the focus effects below would each need a comment naming the dialog close they follow.
|
|
||
| [fix] Fix Tall BottomSheet keyboard layout recovery when iOS interactively dismisses the software keyboard. | ||
|
|
||
| @rubycheung |
There was a problem hiding this comment.
This handle is @rubycheung; your other changesets use @rubyycheung, and the changelog credits the handle written here.
a40cf71 to
d4ea013
Compare
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about the Core BottomSheet change: the on-screen keyboard spec (spec:AST-099) does not say that a Tall sheet's body returns to its pre-keyboard scroll position as the keyboard leaves, so an engineering owner needs to identify or provide the governing spec for that behavior, including the iOS case where only the accessory bar remains, and record the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do this if they are an engineering owner). Independently of that decision, these points need changes:
- Every
scrollevent on the results list blurs the search, including the one a keystroke causes, so typing after the list has scrolled stops after one character (MobileTokenizer.tsx:1169–1171). - Re-checking an item counts as an edit, and Save reports only the latest draft action as
change, which can name an item that never joined or left the value (MobileTokenizer.tsx:142–146,:1098–1104). - A keyboard that only gets shorter while a field stays focused snaps a Tall sheet's body to its opening scroll before gliding back (
useMobileKeyboard.ts:338–346). - The layout effect at
MobileTokenizer.tsx:1038–1046sets state when the sheet closes, the changeset credits@rubycheungrather than@rubyycheung, andspec:AST-099asks for iOS Simulator evidence in a top-level document before keyboard work merges.
[Automated review]
e356461 to
d4ea013
Compare
a25b3a3 to
d4ea013
Compare
User need
People editing mobile Tokenizer selections need search, selection, and dismissal to behave predictably on a phone. The existing Lab flow hid its footer while searching, committed every toggle immediately, and offered no protection against accidental dismissal.
Problem and solution fit
This change updates
MobileTokenizerin Lab to own one selection draft per sheet opening and commit it only from Save. Clear all and Save stay visible while searching. A dirty swipe, scrim press, or Escape opens a discard confirmation; clean dismissals still close directly. Confirmation scrims and Keep editing return to the unchanged draft, with a destructive action followed by a full-width secondary safe action.The large bordered search remains fixed above the results. Scrolling or dragging the results dismisses the software keyboard without clearing the query or interfering with text selection inside the search field.
maxEntriesis enforced against the live draft, including when the limit changes while the sheet is open.The shared Tall
BottomSheetkeyboard logic also restores its pre-keyboard body position as the visual viewport recovers, including accessory-bar-only recovery, while keeping the active field visible and preserving retained layout during dismissal.Public delta and caller burden
MobileTokenizernow:onChangeonce with the final draft and latest change metadata;maxEntriesbefore committing.MobilePowerSearchstring-list and entity-list editors retain their staged outer flow.Design and alternatives
The draft model makes Save meaningful and prevents accidental loss. Confirmation actions stay stacked at full width for touch targets and translation resilience. The destructive action leads; the separated secondary action remains explicit and easy to hit.
Rejected: side-by-side confirmation actions, because they narrow touch targets and place destructive and safe actions closer together.
Evidence
Scope
Testing
pnpm exec vitest run packages/lab/src/MobileTokenizer/MobileTokenizer.test.tsx packages/core/src/BottomSheet/BottomSheet.test.tsxpnpm exec vitest run packages/lab/src/MobilePowerSearch/MobilePowerSearch.test.tsxpnpm --filter @astryxdesign/core buildpnpm --filter @astryxdesign/lab buildpnpm typecheckpnpm check:repopnpm --filter @astryxdesign/storybook build