components icon indicating copy to clipboard operation
components copied to clipboard

chore: Split presentational and logical parts of internal drag handle component

Open avinashbot opened this issue 5 months ago • 1 comments

Description

Big refactor. We previously had a "drag handle button" (which was a basic button with props for pointer/keypress handlers), and a "drag handle wrapper" (which tracked focus states, showed and hid buttons, etc). This turns all the components into simple presentational ones and moves all the wrapper logic into a hook. This refactor will open the door in the future to allow for multiple technical approaches with a simple component.

// Migrating the current logic without major refactors, it looks like this.
const { wrapperProps } = useDefaultDragBehavior(props);
return <DragHandleWrapper {...wrapperProps}><DragHandleButton ... /></DragHandleWrapper>

But in the future, we can support multiple different approaches to dragging, so this is a stepping stone to logic in the future that would look like:

/* HYPOTHETICAL CODE! */

// For a button and slider approach, à la table column resizer
// The hook would be able to automatically track and handle focus between the two buttons
const { wrapperProps, dragButtonProps, sliderButtonProps } = useButtonAndSliderDragBehavior(props);
return (
  <DragHandleWrapper {...wrapperProps}>
    {/* role="button" that listens for an onClick */}
    <DragHandleButton {...dragButtonProps} />
    {/* role="slider" that is focused on keyboard activation */}
    <DragHandleButton {...sliderButtonProps} />
  </DragHandle>
);

// For a plain slider approach, split panel style
const { wrapperProps, dragButtonProps } = useFocusSliderDragBehavior(props);
return (
  <DragHandleWrapper {...wrapperProps}>
    {/* role="slider", nothing too complicated here */}
    <DragHandleButton {...dragButtonProps} />
  </DragHandle>
);

// Board components, sigh...
const { wrapperProps, dragButtonProps } = useTwoDimensionalDragBehavior({ ...props, onDragStart, onDragEnd });
return (
  <DragHandleWrapper {...wrapperProps}>
    {/* role="application" and all the associated handlers */}
    <DragHandleButton {...dragButtonProps} />
  </DragHandle>
);

The "wrapper" as a concept still exists to pass props down to the tooltip or the direction buttons, so it's now basically a compositional convenience:

/* HIGH-LEVEL ABSTRACT CODE! */

<>
  <div ref={ref}>{button}</div>
  <Tooltip trackRef={ref}
  <PortalOverlay track={ref}>
    <DirectionButton direction="..." />
    <DirectionButton direction="..." />
    <DirectionButton direction="..." />
    <DirectionButton direction="..." />
  </PortalOverlay>
</>

See the internal document ztdXAfqVYW3T for more information on dragging interactions.

TODO: Write more.

Related links, issue #, if available: n/a

How has this been tested?

The tests are already there, and since this is a refactor, nothing should break.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

avinashbot avatar Jul 14 '25 15:07 avinashbot

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.61%. Comparing base (22deef7) to head (4251a6d). Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...ts/drag-handle/hooks/use-default-drag-behavior.tsx 92.85% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3651      +/-   ##
==========================================
- Coverage   96.62%   96.61%   -0.01%     
==========================================
  Files         817      818       +1     
  Lines       23804    23781      -23     
  Branches     8346     8345       -1     
==========================================
- Hits        23000    22977      -23     
  Misses        750      750              
  Partials       54       54              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jul 14 '25 15:07 codecov[bot]