Rectangular selection range support (on top of 'next') #1210

rowanhill · 2018-05-26T22:49:59Z

Description

Now that #1123 is complete, I've taken another run at resolving my feature request in #1133 - this PR obsoletes #1158.

This PR adds the ability to select (rectangular) ranges of cells, rather than just a single cell at a time. Both cursor (click and drag) and keyboard (shift + arrow keys) are supported.

I've also added a new example to demonstrate responding to range selection events from a parent component of a ReactDataGrid.

Please check if the PR fulfills these requirements

- [x] The commit message follows our guidelines: https://github.com/adazzle/react-data-grid/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)

I have added a new example page, but I'm not sure if that's sufficient - do I need to update some documentation (e.g. the markdowns in react-data-grid-examples/src/docs)? If so, how are they generated?

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Clicking & dragging selects the text in the cells / shift+arrows just moves the selected cell.

What is the new behavior?
(The below text is similar to, but different from, the text of my old PR.)

Clicking & dragging selects a rectangular range, with the selected/"cursor" cell following the mouse. The range is displayed with a thin border; the selected cell retains a (slightly thicker) border. When the range is greater than 1x1, the selected cells also have a semi-transparent overlay. Text in the grid is no longer selectable except when editing content (in modern browsers; this behaviour won't be supported by IE9 or lower).

The selected range can also be manipulated by holding shift and using the arrow keys. (An existing range is updated by keeping the same start point and moving the "cursor" cell.)

New callbacks (onCellRangeSelectionStarted, onCellRangeSelectionUpdated, and onCellRangeSelectionCompleted) are called (if they're present) when starting a drag, moving whilst dragging, and ending a drag. Modifying the range via the keyboard calls only the latter two callbacks.

There were a few questions I asked in #1133. I've taken the below approaches:

Text selection is prevented in browsers other than IE9 and below. For now, I'm just assuming that's okay, and that this project doesn't support IE9 or lower; I can't see anything official on what's supported, but shout if that's not true.
onViewportDoubleClick doesn't exist (except for outdated docs and specs) on next; it did on master.
onViewportClick has also been removed on next.
Deselecting in general seems to behave slightly differently on next, so I think my original comments on onCellDeSelect aren't relevant anymore.
This feature is always on - i.e. I haven't provided any setting to disable it. (I haven't looked into the behaviour of enableCellSelect on next.)

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

As before, I think the closest thing to a breaking change is that text isn't selectable any more.

Other information:

My previous PR had a first commit that refactored some tests (because without that refactoring, I couldn't get them to run reliably). I haven't needed them this time around, but I did rebase them, so I can easily put up another PR to add them to next if anyone's interested.

Parent components of ReactDataGrid can subscribe to (start, update, end) events about selection ranges. An example page demonstrates this.

rowanhill · 2018-06-12T16:33:26Z

@amanmahajan7 Any news on this / when someone could take a look?

Sorry to pester, but I don't want the code to move on again and mean I have to rework and resubmit this PR yet again! :)

stale · 2018-07-12T16:34:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please reopen this if you feel it has been incorrectly closed and we will do our best to look into it. Thank you for your contributions.

rowanhill · 2018-07-15T06:38:16Z

@amanmahajan7 Is this really wontfix? It seemed like there was interest in getting it merged, before?

malonecj

Hi @rowanhill. Thanks for the submission. I checked the example and it works great. There are a couple of things would like to see added before we merge.

I've added some comments below, and will add one or two more in the morning, but overall looking really good.

Also, I would like to see some tests around new functionality as well

malonecj · 2018-07-18T16:31:22Z

packages/react-data-grid/src/masks/InteractionMasks.js

+    this.selectCell(next);
+  }
+
+  changeCellFromTabAction(e) {


changeCellFromArrowKeyAction and changeCellFromTabAction are practically the same function. It should be consolidated to changeCellFromArrowKeyAction, passing in the relevant cellNavigationMode as a parametre

Okay, sure.

malonecj · 2018-07-18T16:34:06Z

packages/react-data-grid/src/masks/InteractionMasks.js

+  changeCellFromArrowKeyAction(e) {
+    const currentPosition = this.state.selectedPosition;
+    const { cellNavigationMode } = this.props;
+    const next = this.getNextPositionAndFireOnHitBoundaryIfNeccesary(e, currentPosition, cellNavigationMode);


The name of this function suggests it has too many responsibilities
Better change this to

const next = this.getNextPosition(e, currentPosition, cellNavigationMode); this.checkCellIsAtGridBoundary();

Okay, I've made a change along the lines you suggested.

It's involved pulling out creating the keyNavAction too (so it can be passed to both of these functions, rather than needlessly created twice), plus the check function needs a couple of other params passing to it.

Personally, I'm not sure this is an improvement for legibility, but I'm happy enough with it if you prefer it this way.

malonecj · 2018-07-18T16:34:50Z

packages/react-data-grid/src/masks/InteractionMasks.js

+    this.selectUpdate(next, true, () => { this.selectEnd(); });
+  }
+
+  getNextPositionAndFireOnHitBoundaryIfNeccesary(e, currentPosition, cellNavigationMode) {


change function name to getNextPosition as described above

malonecj · 2018-07-18T16:37:48Z

packages/react-data-grid/src/masks/InteractionMasks.js

      }, callback);
    }
  };

+  selectStart = (cellPosition) => {


Should be more meaningful name like onCellRangeSelectionStarted

Done, along with similar methods.

malonecj · 2018-07-18T16:41:06Z

packages/react-data-grid/src/masks/InteractionMasks.js

+      if (isFunction(this.props.onCellRangeSelectionStarted)) {
+        this.props.onCellRangeSelectionStarted(this.state.selectedRange);
+      }
+    });


function could be simplified using shorthand notation

onCellRangeSelectionStarted(selectedPosition) { this.setState({ selectedRange: getSelectedRange(selectedPosition), selectedPosition }, () => { if (isFunction(this.props.onCellRangeSelectionStarted)) { this.props.onCellRangeSelectionStarted(this.state.selectedRange); } }) };

Okay. I made a member function createSingleCellSelectedRange and I'm taking isDragging as a parameter so it's slightly more reusable.

malonecj · 2018-07-18T16:51:45Z

packages/react-data-grid/src/masks/InteractionMasks.js

-        {!isEditorEnabled && this.isGridSelected() && (
-          <SelectionMask
-            selectedPosition={selectedPosition}
+        {selectedRangeIsSingleCell(this.state.selectedRange) ? (


I think it could be nice to split this chunk of jsx into simple sub components for readability and to simplify the conditional logic.

{selectedRangeIsSingleCell(this.state.selectedRange) ? <SingleCellSelectView {...relavantProps/> : <CellRangeSelectView {...otherRelevantProps}/>

I looked into pulling out components in separate files, but it wasn't worth it - there were so many props that needed passing that it wasn't actually an improvement. Also, your CellRangeSelectView component would need to return (an array of) multiple components when it renders, which is only supported in React 16.

Instead, I've created a couple of class member functions to generate the JSX snippet / array. It doesn't look quite as pretty as components, but it does look cleaner than it was, and it should work with older versions of React.

rowanhill · 2018-07-19T21:30:08Z

Thanks for the comments - I've made some changes.

Re "Also, I would like to see some tests around new functionality as well" - there are quite a number of tests in InteractionMasks.spec.js that I think exercise the fiddliest bits of the code, could you let me know more specifically what sort of tests you're thinking of?

malonecj · 2018-07-23T14:13:58Z

packages/react-data-grid/src/masks/InteractionMasks.js

-          </SelectionMask>
-        )}
+        {selectedRangeIsSingleCell(this.state.selectedRange) ?
+          this.getSingleCellSelectView(rowData) :


Our preference is to break render method up into smaller components, rather than splitting into smaller render methods. Even if the two functions here wont be reused outside of this component, splitting into smaller components is a good way to decompose fat components, and allows us to easily identify which props and state the rendered components use.

It also allows to easily isolate those units for independent testing.

https://medium.com/dailyjs/techniques-for-decomposing-react-components-e8a1081ef5da

Did you see my previous comment about pulling out components rather than render methods?

I'll do it if you're sure, but I don't think it's a great idea: the components themselves end up being extremely simple (so there's little value in testing them), and take so many props the JSX ends up looking just as bad, if not worse, than before.

Additionally, one of these render methods returns an array of elements, which isn't possible with a single component in anything but the most recent versions of React.

As a rule of thumb I would keep the new components in the same file if they are not going to be reused. I thought with destructuring it would look clean to pick the relevant props, and then pass them in with spread syntax.

But if you tried this and it didn't look so neat, I'm happy to go with this solution. I'm not sure if anyone is using this with earlier versions of React. Unofficially, only React 16 is supported in v4, but the docs should be explicit about this.

Anyway, this looks good to me

malonecj

The changes here look good.
And I am happy with the level of tests. It was mostly around the rendering and behaviour of the CellRangeSelection so looks like this is well covered.

I Am only suggesting two changes. One is mentioned in a comment above regarding component decomposition of InteractionMask

The second is actual api to ReactDataGrid

Since the amount of props that the grid can now take is getting really large, I think it would be cleaner to start grouping related groups. So instead of

  onCellRangeSelectionStarted={this.onStart}
  onCellRangeSelectionUpdated={this.onUpdate}
  onCellRangeSelectionCompleted={this.onComplete}

we could do

  cellRangeSelection={onStart:this.onStart, onUpdate: this.Update, onComplete: this.onComplete}

…lRangeSelection prop

rowanhill · 2018-07-24T21:46:17Z

Okay, I've pushed those changes to the API you requested, plus I've also refactored the selection masks into full-on components. I'm still not 100% convinced by that change, but now you can take a look for yourself and decide which you prefer! I've isolated that change in a commit, so we can just revert it if you'd prefer.

rowanhill · 2018-07-25T08:21:15Z

Oh, and just to be super explicit: that selection mask component refactoring does include a component that returns an array of elements, so it does definitely make this library React 16+ only - so you'll need to be sure that's okay!

malonecj · 2018-07-25T08:45:10Z

I see what you mean. I thought it would be less versbose to pass the props in. Really appreciate your effort on all this. Let's remove the last commit and its good to merge in.

malonecj

Remove last commit and good to go!!

…ponents" This reverts commit 8ea34d7.

rowanhill · 2018-07-25T18:22:21Z

Okay, I've pushed a revert of that commit, so hopefully that's all sorted now.

* Allow for optional SimpleCellFormatter.value prop (#948) * Improve Cell navigation Performance (#1123) * Move cell selection responsibilty to SelectionMask component * Selection Mask working at 60fps * Tests for InteractionMask navigation * Editor Container in interaction Layer * commit package-lock.json * Editing Working * smooth scrolling with keyboard navigation * Adjust visible start/end calculations after scroll * Vertical cell Scrolling with keyboard working * generic move command * Vertical scrolling navigation with keyboard working * Code cleanup - Remove extra ref - Format JSX * Update react to the stable version Delete/Ignore package-lock.json * Remove deprecated babel plugins * Export individual constants instead of exporting a default object * Remove react dependency from react-data-grid Delete package.json for now * Remove ReactDOM reference and use refs instead * More code cleanup - Fix formatting - Remove extra spread operator * Add innerRef to fix scrolling Remove unused methods * Remove GridWrapper and GridContainer as it breaks instance methods * Remove unused CellExpander component * - Remove singleton store - Add basic implementation for store/reducer/dispatch - Remove CellContainer to prevent rerendering of each cell on selection change * Delete unused files Cleanup event handlers * Move copy paste functionality to the Interaction mask component Check if cell is editable * Remove flow types * Remove flow types * Remove unused variables * Move copy/paste logic to the interaction layer * Horizontal scroll using keyboard Temporarily disable animation * Rename folder to containers * Fix cell navigation mode * Do not prevent event bubling to show context menu * Add pointer events to fix cell actions * Fix Filter Row focus * Dynamically connect cell to support custom renderers * Fox committing changes Disable cell container component as it degraded performance * Migrate cell drag down to Interaction Layer * Remove react-motion * Add CellMask to highlight copied cell * Add drag mask to highlight dragged cells * Cleanup render method * Add eventbus to communicate with InteractionMask component Delete state and containers * Move EventTypes to contants folder * Fix clicking on a cell scrolls the canvas to the top * Fix cell double click to edit * Fix tests InteractionMasks * Fix issue with press enter not committing * Keep InteractionMask focus logic in central place * Fix canvas tests * Fix Canvas tests * Remove redundant Cell tests * Fix Cell tests and remove redundant ones * Fix clicking on a cell scrolls the canvas to the left Remove componentWillMount * Add a new DragEnter method Some code cleanup * Delete redundant displayStart and displayEnd states * Prevent navigation in FF on drag * Fix Grid.js and remove redundant tests * Move tab behaviour from ReactDataGrid to InteractionMask * Clean up initial tests for tab behavior in InteractionMasks * Fix typo and event name * Use CellNavigationMode constant * Ignore invalid drag items * Use a single div instead of multiple divs for drag mask * Fix SelectionMask zIndex for locked columns * Delete OverflowCell and OverflowRow components * Remove unused props and methods * Clean up props * Fix scrolling when scrollToRowIndex is set * Reorder props for clarity * Commented unused props and methods * Fix ESLint errors * Dispatch SCROLL_TO_COLUMN event to scroll Canvas from ReactDataGrid * Trigger onCellSelected, onCellDeSelected events when cell selection is changed Fix openCellEditor instance method * Fix openCellEditor instance method * Specify required PropTypes * Fix closing tags * Enable auto focus on grid * Fix last row check * Implement tab behavior for InteractionMasks * Fix PropTypes warnings Remove unused state * Fix Cell PropTypes * Fix rowData in EditorContainer * Fix null reference error for drag copy * Check onCheckCellIsEditable for editing * Fix some broken unit tests * Removed redundant and fixed remaining tests * Fix context menu * Fix RowsContainer unit tests * Disable duplicate tests * Fix example home page fonts * Install latest node version * Simplify appveyor script after node upgrade * Fix lint errors * Fix failing IE 11 test * IE compatible version of Array.keys * packages/rdg-test-utils/index.js * Delete unused test-utils project * install gulp gloablly in order to post to coveralls * Use node 6 on appveyor * Delete obsolete scroll utils * Add unit tests for CellMask * Add unit tests for CopyMask * Add unit tests for DragMask * Remove fdescribe * Add unit tests for DragHandle * Add unit tests for EventBus * Format code and object initialization cleanup * Add unit test for copy operation * Use conditional rendering instead of returning null * Add unit tests for Drag functionaity * Add unit tests for the SelectionMask component * Add unit tests for CanvasUtils * Add unit tests for context menu * Add test to check if Canvas renders the InteractionMasks component * Delete commented code * Handle scrolling when selected cell hits the top/bottom boundary when the cellNavigationMode is changeRow * Fix tab behavior * Fix cell selection mask for grouped rows * Add back IE support * Add polyfills for Set and Map to make the examples work in IE10 * Remove unused variable * Fix zIndex for filters * Fix scrolling when editor is closed * Deleted commented code * Fix eslint errors * Delete obsolete tests Remove commented code * Fix Typo * Move the drag enter logic to Row component * Uncomment code * Fix event bus prop type * Define ref callback as a bound method on the class * Remove private getSelectedValue method * Fix Cell component export and columns count (#1227) * Fix cell export * Fix columns count * update file names (#1245) * Rectangular selection range support (on top of 'next') (#1210) * Allow selection of ranges (with mouse & keyboard) Parent components of ReactDataGrid can subscribe to (start, update, end) events about selection ranges. An example page demonstrates this. * Minor InteractionMasks refactoring based on comments on PR #1210 * Update ReactDataGrid API to group cell range event handlers under cellRangeSelection prop * Refactor single and multi-cell range selection masks into components * Revert "Refactor single and multi-cell range selection masks into components" This reverts commit 8ea34d7. * Scrolling improvements (#1254) * Effiecient windowing example * Remove use of refs to setScrollLeft. Renamed row/col visible boundary property names * Calculate overscan columns based on scroll direction * Render overscan columns and rows based on scroll direction * Set height of grid * Improved overscan row/col calcualtion * Send isScrolling prop to formatters to improve perf of complex formatters * Pass isScrolling prop to HeaderCell and Formatter * Pass isScrolling prop to HeaderCell and Formatter 2 * Set pointer-events to none when scrolling * reset setScrollLeft functionality to enable frozen columns * Ensure set scroll left on frozen columns when scrolling in vertical direction * Fix context menu * zindex styles for locked cells * Render overscan columns when row scrolling has complete * Show correct SelectionMask position for locked cells * is scrolling example * Do not deep compare dependentValues check * Ensure rowOverscanEndIdx within bounds * Ensure frozen cells are mounted in correct fixed position if canvas has been scrolled * Move InteractionMasks styles to a separate file * Cherry pick set cell ref commit * Base SelectionMask top and left off rendered cells to handle dynamic row heights * Take scroll left into account when open editor container * Catchup with next branch * Re-add lost canvas.js file * Fix lint errors * Fix some build errors * Remove references to deleted RowsContainer file * Fix typo * Fix InteractionMaks test * Fix some tests * Fix failing tests * Handle dynamic row heights in SelectionMask * Remove unnecessary packages * Viewport scroll calculations * Address pull request comments * use let instead of const in checkScroll function * Remove unneeded id from Row component * Chang logic around getRenderedColumns * Address more PR comments * Tests for Viewport Utils * Fix lint errors and remove console rule from lintrc * Tests for getScrollDirection * Tests for getRowOverscanStartIdx * getRowOverscanEndIdx tests * Tests for getColOverscanEndIdx * Tidy up syntax in example31-isScrolling * Ensure locked columns are shifted to start of column metrics array * Test for rendering the correct cells * Fix lint errors * Rename all fixed/locked/frozen columns to frozen * Fix tests after npm update * Fix IE test * Fixed new warnings in tests * Fix failing test * Update version to 5.0.0-alpha1 * Ensure render all visible columns if scrolled to the right and using frozen columns * Version bump * Change test expectation * Fix test * Ensure last column is rendered when grid scrolled and using frozen columns * Minor version bump * Handle viewport scroll state when no columns present * 5.0.0-alpha8 * Fallback for getSelectedRowTop if no access to row ref * Fix some bugs around cell selection for RowGroups * Fix failing test * Pass row prop to formatters * Fix failing test and version bump * Fix lint error * Re-add deprecated onRowUpdatedMethod - to be removed in v6 * Address some more PR comments * Air BNB browser shims for Enzyme testing * Remove unused functions * prop deprecation warnings * onDragHandleDoubleClick deprecation warning * Publish next branch on commit to next * Get selected row columns if applicable * Fix failing tests * Update npmrc token * Fix some zIndex issues around frozen cells and editors * Remove ReactDataGrid dependency from addons bundle (#1272) * Fix some zIndex issues around frozen cells and editors * Remove rdg dependency from addons package * Fix object assign * Adress PR comments * Fix tests * Fix lint error * add ref to draggable header cell * Adress PR comments * Pass renderBase row and correct ref to custom row renderer * Change editor container to be fixed position * Add missing rdg-editor-container class on EditorContainer (#1275) * Ensure row refs reference an RDG Row component * Show all columns unless scrolling vertically * isFrozen column check * Render edtior container without taking scrollLeft into account if column is frozen * Set CellMask position to be fixed when scrolling horizontally and frozen column is selected * Unit tests and refactoring (#1279) Added unit tests for editor position relating to frozen columns. And refactored spec file, updating legacy code * Upgrade to RDG 5

xdev-x · 2023-08-19T16:23:05Z

What is the status on this PR? indicated as merged, but it seems it has never been merged to main :(

Allow selection of ranges (with mouse & keyboard)

7794aba

Parent components of ReactDataGrid can subscribe to (start, update, end) events about selection ranges. An example page demonstrates this.

rowanhill mentioned this pull request May 26, 2018

Rectangular selection range support #1158

Closed

stale bot added the wontfix label Jul 12, 2018

stale bot removed the wontfix label Jul 15, 2018

malonecj suggested changes Jul 18, 2018

View reviewed changes

Minor InteractionMasks refactoring based on comments on PR adazzle#1210

d97094d

malonecj suggested changes Jul 23, 2018

View reviewed changes

rowanhill added 2 commits July 24, 2018 21:14

Update ReactDataGrid API to group cell range event handlers under cel…

80e49e1

…lRangeSelection prop

Refactor single and multi-cell range selection masks into components

8ea34d7

malonecj approved these changes Jul 25, 2018

View reviewed changes

malonecj suggested changes Jul 25, 2018

View reviewed changes

Revert "Refactor single and multi-cell range selection masks into com…

dc689d7

…ponents" This reverts commit 8ea34d7.

malonecj approved these changes Jul 26, 2018

View reviewed changes

malonecj merged commit 5d76eb9 into adazzle:next Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rectangular selection range support (on top of 'next') #1210

Rectangular selection range support (on top of 'next') #1210

rowanhill commented May 26, 2018

rowanhill commented Jun 12, 2018

stale bot commented Jul 12, 2018

rowanhill commented Jul 15, 2018

malonecj left a comment

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

malonecj Jul 18, 2018

rowanhill Jul 19, 2018

rowanhill commented Jul 19, 2018

malonecj Jul 23, 2018

rowanhill Jul 23, 2018

malonecj Jul 24, 2018

malonecj left a comment

rowanhill commented Jul 24, 2018

rowanhill commented Jul 25, 2018

malonecj commented Jul 25, 2018

malonecj left a comment

rowanhill commented Jul 25, 2018

xdev-x commented Aug 19, 2023

Rectangular selection range support (on top of 'next') #1210

Rectangular selection range support (on top of 'next') #1210

Conversation

rowanhill commented May 26, 2018

Description

rowanhill commented Jun 12, 2018

stale bot commented Jul 12, 2018

rowanhill commented Jul 15, 2018

malonecj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanhill commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malonecj left a comment

Choose a reason for hiding this comment

rowanhill commented Jul 24, 2018

rowanhill commented Jul 25, 2018

malonecj commented Jul 25, 2018

malonecj left a comment

Choose a reason for hiding this comment

rowanhill commented Jul 25, 2018

xdev-x commented Aug 19, 2023