Add cellPosition and onCellPositionChange props#420
Conversation
f2f4b49 to
f1e553f
Compare
f1e553f to
f96f419
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds controlled component support for cell navigation by introducing cellPosition and onCellPositionChange props to the HighTable component. This replaces the previous PR #396's approach of using custom event targets, providing a more idiomatic React pattern for parent components to control which cell is active.
Changes:
- Adds
cellPositionandonCellPositionChangeprops following the controlled/uncontrolled component pattern - Integrates these props into CellNavigationProvider using the existing
useInputStatehook - Updates documentation to clearly explain controlled vs uncontrolled modes and lifecycle expectations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds CellPosition prop definitions with comprehensive documentation for controlled/uncontrolled modes |
| src/providers/CellNavigationProvider.tsx | Integrates cellPosition props with useInputState hook; updates focus state handling |
| src/components/HighTable/HighTable.tsx | Threads cellPosition and onCellPositionChange props through to CellNavigationProvider |
| src/providers/ScrollProvider.tsx | Removes outdated parameter documentation comment |
| src/helpers/scroll.ts | Removes completed TODO comment |
| src/components/HighTable/HighTable.stories.tsx | Adds JumpToCell story demonstrating controlled cell navigation with buttons |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button type="button" onClick={() => { setCellPosition({ rowIndex: 500, colIndex: 3 }) }}> | ||
| Go to row 500, column 3 | ||
| </button> | ||
| <button type="button" onClick={() => { setCellPosition({ rowIndex: 100_000_000, colIndex: 2 }) }}> |
There was a problem hiding this comment.
The button label says "column 4" but the code sets colIndex: 2. The colIndex should be 4 to match the label, or the label should say "column 2" to match the code.
| <button type="button" onClick={() => { setCellPosition({ rowIndex: 100_000_000, colIndex: 2 }) }}> | |
| <button type="button" onClick={() => { setCellPosition({ rowIndex: 100_000_000, colIndex: 4 }) }}> |
| {`Go to row ${data.numRows + 1}, column 3`} | ||
| </button> | ||
| </div> | ||
| {/* focus is broken */} |
There was a problem hiding this comment.
The comment "focus is broken" suggests there is a known issue with the feature being demonstrated in this story. This should either be fixed before merging, or if it's a temporary comment for development purposes, it should be removed. If it's a known limitation, it should be documented more clearly explaining what specifically is broken and why.
| {/* focus is broken */} | |
| {/* Note: changing cellPosition scrolls to the target cell but does not move browser keyboard focus. */} |
| ) | ||
| }, | ||
| args: { | ||
| // data: createUnsortableData(), |
There was a problem hiding this comment.
The commented out code should either be removed or have a clear explanation of why it's kept. Dead code that's commented out makes the codebase harder to maintain. If different data sources are needed for testing, consider using Storybook controls or multiple story variants instead.
| // data: createUnsortableData(), |
platypii
left a comment
There was a problem hiding this comment.
tested locally, looks good!
This gives the parent component a way to control cell navigation:
Screencast.From.2026-02-05.22-43-10.mp4
This replaces #396.
I'll not add tests for now, because we already have tests for the internal logic, and the stories work well.
But, mostly, because we cannot test the scroll with jsdom. See #423.