Repository navigation
Add exclusive groups for dialogs #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,8 @@ export function useScanItem( | |
|
|
||
| export interface ScanGroupOptions { | ||
| order?: number; | ||
| /** Confine scanning to this group while it is mounted, as for an open dialog. */ | ||
| exclusive?: boolean; | ||
| } | ||
|
|
||
| export interface ScanGroupState { | ||
|
|
@@ -224,14 +226,14 @@ function groupState(key: GroupKey): ScanGroupState { | |
| /** Registers a group in the current scope. Wrap its children in ScanGroupScope. */ | ||
| export function useScanGroup( | ||
| id: string, | ||
| { order }: ScanGroupOptions = {}, | ||
| { order, exclusive }: ScanGroupOptions = {}, | ||
| anchor?: unknown, | ||
| ): ScanGroupState { | ||
| const controller = useScanController(); | ||
| const scope = useContext(ScopeContext); | ||
| useEffect( | ||
| () => controller.registerGroup(id, { parentId: scope.id, order }, anchor), | ||
| [controller, id, scope.id, order, anchor], | ||
| () => controller.registerGroup(id, { parentId: scope.id, order, exclusive }, anchor), | ||
| [controller, id, scope.id, order, exclusive, anchor], | ||
|
Comment on lines
+235
to
+236
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing Preserve exclusive priority across option updates and add a regression test for an older dialog updating while a newer one remains open. This must be fixed before merging. Artifacts
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/react/index.tsx
Line: 235-236
Comment:
**Older dialog steals scanning**
Changing `order` on an older exclusive `ScanGroup` takes scanning away from a newer, still-open dialog. The registration effect cleans up the older group, deleting its exclusive priority, then registers it with a higher priority. The next Select activates a control in the older dialog even though the newer dialog remains mounted and exclusive.
Preserve exclusive priority across option updates and add a regression test for an older dialog updating while a newer one remains open. This must be fixed before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| ); | ||
| const read = useCallback( | ||
| (snapshot: ScanSnapshot): GroupKey => { | ||
|
|
@@ -261,8 +263,8 @@ export interface ScanGroupProps extends ScanGroupOptions { | |
| } | ||
|
|
||
| /** A group with no element of its own. Use the dom or native wrappers to order groups by layout. */ | ||
| export function ScanGroup({ id, order, children }: ScanGroupProps) { | ||
| const state = useScanGroup(id, { order }); | ||
| export function ScanGroup({ id, order, exclusive, children }: ScanGroupProps) { | ||
| const state = useScanGroup(id, { order, exclusive }); | ||
| return ( | ||
| <ScanGroupScope id={id}>{typeof children === "function" ? children(state) : children}</ScanGroupScope> | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclusive group's ID disappears from
ScanSnapshot.pathbecause the new tree starts at its children.useScanGrouptherefore reportsentered: falseeven while scanning highlights and successfully activates an item inside that group. This is a non-blocking state-reporting defect: callers usingenteredfor styling or feedback do not show that scanning is inside the dialog.Include the exclusive scope in the reported path without adding a way to escape it.
Artifacts
Evidence from the check
Command output from the check
Command output from the check
Prompt To Fix With AI