feat(Group): pass isUserInteraction flag to onLayoutChanged (closes #716)#721
Conversation
|
Someone is attempting to deploy a commit to the Brian Vaughn's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@bvaughn can you review the changes? |
|
I'm still thinking about whether I want to land this change. Only reporting pointer-initiated resizes as "user interactions" is misleading. Keyboard interactions are also user interactions. Arguably imperative API methods too in some cases though I think it's not really possible for this library to attribute those. |
kenanyildiz
force-pushed
the
pr/716-isUserInteraction
branch
from
e0e220a to
3b0b5e2
Compare
|
You're right about keyboard — that was an oversight, fixed in 3b0b5e2. Both pointer and keyboard resizes originate from a real DOM event on the separator, so the library can confidently attribute them. adjustLayoutForSeparator (the only keyboard dispatch site) now passes isUserInteraction: true, covering arrow keys, Home/End, and Enter collapse/expand. For the imperative API (setLayout, etc.) I'd keep it false. As you noted, the library can't attribute intent there — a setLayout() call could come from a user clicking a custom button or from purely automated logic, so flagging it would risk false positives. The consumer already knows when they invoked the method and can attribute intent on their side. This keeps the flag's meaning precise — "the user directly manipulated a separator." I've updated the onLayoutChanged JSDoc to document the boundary and adjusted the keyboard test to expect true. |
) Per review feedback, replace the raw `isUserInteraction` boolean with a `LayoutChangedMeta` object (`{ isUserInteraction }`). This makes the call sites self-documenting and leaves room to add further metadata later without another signature change. Export `LayoutChangedMeta` from the public entry point and update Group, useDefaultLayout, and the affected tests.
|
Done in b7ad932 — wrapped the second argument in a LayoutChangedMeta object ({ isUserInteraction }), so the call sites are self-documenting and we can extend the metadata later without another signature change. Also exported the type from the public entry point. Updated useDefaultLayout and all affected tests; types and the full group/useDefaultLayout suites pass. |
…hn#716, bvaughn#721) `onLayoutChanged` now receives a second argument: a `LayoutChangedMeta` object whose `isUserInteraction` field is `true` only when the user directly manipulated a separator — releasing a pointer drag (including the missed-pointerup fallback) or pressing a resize key (arrow keys, Home/End, Enter collapse/expand). Both originate from a real DOM event, so the library can attribute them. It is `false` for every other source — initial mount, programmatic `setLayout` and other imperative API calls, constraint recompute, and default-size changes — because the library cannot attribute the caller's intent there. The flag is wrapped in a meta object (rather than a raw boolean) so call sites are self-documenting and further metadata can be added later without another signature change. `LayoutChangedMeta` is exported from the public entry point. `useDefaultLayout` persists every commit regardless of the flag, since it owns its storage; consumers that only want to save on user interaction should branch on `meta.isUserInteraction` in their own callback. Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
kenanyildiz
force-pushed
the
pr/716-isUserInteraction
branch
from
b7ad932 to
44d1f63
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
Assuming the tests pass, this looks good. Thanks for your patience.
| // because it owns its own storage and the goal is to remember whatever | ||
| // layout the user is currently looking at. Consumers that only want to | ||
| // persist on user interaction should branch on `isUserInteraction` in | ||
| // their own callback (see #716) rather than via this hook. |
There was a problem hiding this comment.
I think we should probably add an option to this hook to allow this behavior to be configured, but I can do that as a follow on.
2 participants
Closes #716.
Scope
This is the small-bool variant agreed in the issue thread:
onLayoutChanged: (layout, isUserInteraction: boolean) => voidtrueonly at pointer-up dispatches — the canonical end-of-drag site inonDocumentPointerUpand the missed-pointerup fallback (Still resizes after releasing mouse outside window #340) inonDocumentPointerMove.falsefor every other source: initial mount, programmaticsetLayout, parent re-render with freshdefaultSize, constraintclamp due to container width change, keyboard arrow-key resize.
The signal is threaded through
GroupChangeEventandupdateMountedGroup; theGroupsubscriber forwardsevent.isUserInteractionto theonLayoutChangedStablewrapper.Open question — required vs optional
I went with
isUserInteraction: boolean(always defined) rather than the?: booleansuggested in the comment thread because consumers can writeif (isUserInteraction)withoutboolean | undefinednarrowing and the emit path stays symmetric. Strictly, this widens theOnGroupLayoutChangedtype, so users who annotated their handler with the strict 1-arg signature would need a 1-line fix (or just accept the second arg). JS runtime is fully backwards-compatible since extra arguments are ignored. Happy to flip to optional if you'd prefer.Out of scope (called out for transparency)
falsehere. Treating keyboard as a user interaction would be a follow-up in the separator keydown handler; defer to you on whether to bundle.useDefaultLayoutneeded a small adaptation to compile against the newOnGroupLayoutChangedtype (its returned callback's signature has to match the prop). Included as the second commit. The hook persists on every commit as before — consumers that want user-only persistence can branch on the flag in their own callback.Test plan
Group.test.tsxpointer-up tests updated to assert(layout, true).(layout, false).useDefaultLayout.test.tsxassertions updated to the new two-argsignature.