[Table] Support for column and row reordering #963

cmslewis · 2017-04-07T19:40:36Z

Fixes #213

Checklist

Include tests
~~Update documentation~~ (will add language in a separate PR if we want it)
Add new example
Fix interaction regression w/ TruncatedPopover

Changes proposed in this pull request:

Behold: column and row reordering in Table.

Basic points:

Reordering works only if selection is also enabled. If this assumption is bad, let me know.
Reordering is performed by mousing down within a selected column header, then dragging. If reordering is enabled, it is no longer possible to deselect a column by clicking on its header. LMK how that feels; I don't love it yet.
You can enable reordering for columns, rows, or both via Table props.
You can reorder a single entity or a region of multiple entities (having FULL_COLUMNS or FULL_ROWS cardinality).
You CANNOT reorder anything if there are multiple regions selected. It's too much of a pain for a v1 implementation, plus initial feedback suggested that no user will be able to predict what will happen anyway. We can revisit this later if we want.
The Table does not handle the actual reordering! It simply emits (oldIndex, newIndex, length) and lets the caller pass in updated data.

Reviewers should focus on:

High-level architecture. Does it look reasonable?
One thing that felt a little gross was the introduction of a "reordering cursor overlay" that displays invisibly over the whole table while you're dragging to reorder. I couldn't think of a better way to guarantee that the cursor would appear as grabbing no matter where your mouse moved during the drag. LMK what you think.

Screenshot

Should undo before merging

Use it in DragSelectable to prevent DragReorderable from triggering its handleActivate on the same mousedown event.

…plus tests)

cmslewis · 2017-04-12T22:47:13Z

I'm also seeing a lot of duplicated functionality between RowHeader and ColumnHeader. We should pull the common code into an AbstractHeader component, e.g. Would prefer to do that in a separate PR.

blueprint-bot · 2017-04-12T22:52:23Z

Bugfix: enable reordering right away if selection exists on mount

Preview: documentation | landing | table
Coverage: core | datetime

blueprint-bot · 2017-04-12T23:36:51Z

Fix tests and increase coverage to 79% again

Preview: documentation | landing | table
Coverage: core | datetime

giladgray · 2017-04-13T01:14:19Z

packages/table/preview/index.tsx

+    country: array[3],
+    city: array[4],
+    // tslint:enable:object-literal-sort-keys
+}));


i mean you might as well go hard...

([letter, fruit, ...]) => ({ letter, fruit, ... })

giladgray · 2017-04-13T01:15:34Z

packages/table/preview/index.tsx

+class ReorderableTableExample extends React.Component<{}, IReorderableTableExampleState> {
+    public state = {
+        data: REORDERABLE_TABLE_DATA,
+    } as IReorderableTableExampleState;


always better to declare type up front instead of casting. sometimes you lose type safety when casting, which you don't lose if you type the var first.

public state: IReorderableTableExampleState = { ... }

giladgray · 2017-04-13T01:17:26Z

packages/table/src/headers/columnHeader.tsx

        isResizable: true,
        loading: false,
    };

+    public state = {
+        hasSelectionEnded: false,
+    } as IColumnHeaderState;


again, do it up front. and on one line.
public state: IColumnHeaderState = { hasSelectionEnded: false };

giladgray · 2017-04-13T01:19:48Z

packages/table/src/headers/columnHeader.tsx

+        return Regions.column(index1, index2);
+    }
+
+    private isColumnTemporarilyReorderable(isColumnSelected: boolean) {


what is this for? what do these five conditions mean? code comment please

giladgray · 2017-04-13T01:21:20Z

packages/table/src/headers/columnHeader.tsx

@@ -168,50 +205,98 @@ export class ColumnHeader extends React.Component<IColumnHeaderProps, {}> {
        });
        const cellLoading = cell.props.loading != null ? cell.props.loading : loading;
        const isColumnSelected = Regions.hasFullColumn(selectedRegions, columnIndex);
-        const cellProps: IColumnHeaderCellProps = { className, isColumnSelected, loading: cellLoading };
+        const isColumnTemporarilyReorderable = this.isColumnTemporarilyReorderable(isColumnSelected);


does it matter that it's "temporary?" i think "currently" might be a better word, cuz it's determined by props and state. but that's just pure react, so perhaps we don't need such a word at all.

usage doesn't suggest any notion of temporary--you immediately rename it to isColumnReorderable, which as a prop is understood to only matter in the now, not next render.

Yeah wasn't sure about this, but I wanted to more strongly disambiguate isReorderable from isColumnReorderable. It's not necessarily clear at a glance what the difference is.

giladgray · 2017-04-13T01:22:19Z

packages/table/src/headers/rowHeader.tsx

@@ -161,45 +197,93 @@ export class RowHeader extends React.Component<IRowHeaderProps, {}> {
        });
        const cellLoading = cell.props.loading != null ? cell.props.loading : loading;
        const isRowSelected = Regions.hasFullRow(selectedRegions, rowIndex);
-        const cellProps: IRowHeaderCellProps = { className, isRowSelected, loading: cellLoading };
+        const isRowTemporarilyReorderable = this.isRowTemporarilyReorderable(isRowSelected);


again with the temporary

giladgray · 2017-04-13T01:25:25Z

packages/table/src/interactions/selectable.tsx

+    private getDragSelectedRegions(event: MouseEvent, coords: ICoordinateData) {
+        let region = (this.props.allowMultipleSelection) ?
+            this.props.locateDrag(event, coords) :
+            this.props.locateClick(event);


condition ? true : false;

giladgray · 2017-04-13T01:28:20Z

packages/table/src/regions.ts

@@ -548,11 +541,52 @@ export class Regions {
        }
    }

+    private static overlapsRegionInternal(regions: IRegion[], query: IRegion, allowPartialOverlap = true) {


👎 why can't this just be overlapsRegion? the third prop is optional with a default anyway.
i don't see the need for both.

giladgray · 2017-04-13T01:29:08Z

packages/table/src/regions.ts

    private static regionsEqual(regionA: IRegion, regionB: IRegion) {
        return Regions.intervalsEqual(regionA.rows, regionB.rows)
            && Regions.intervalsEqual(regionA.cols, regionB.cols);
    }

+    private static regionContains(regionA: IRegion, regionB: IRegion) {
+        // containsRegion expects an array of regions as the first param
+        return Regions.overlapsRegionInternal([regionA], regionB, /* allowPartialOverlap */ false);


i don't think we need the prop name comment in typescript. i can easily see it by hovering on function name.

blueprint-bot · 2017-04-13T01:36:48Z

Update hasSelectionEnded on componentWillReceiveProps

Preview: documentation | landing | table
Coverage: core | datetime

cmslewis · 2017-04-13T02:14:56Z

@giladgray - I fixed the stuff in your CR just now.

blueprint-bot · 2017-04-13T02:17:51Z

Respond to Gilad CR feedback

Preview: documentation | landing | table
Coverage: core | datetime

adidahiya · 2017-04-13T05:02:04Z

packages/table/src/headers/columnHeader.tsx

+     * Enables/disables the reordering interaction.
+     * @default true
+     */
+    isReorderable?: boolean;


I agree with @themadcreator, this should not be exposed at this level if we don't expect to support it. How about if you make it @internal, does that work? If not, you could try to use component context to allow <Table> to talk to its sub components.

adidahiya · 2017-04-13T05:05:05Z

packages/table/src/interactions/reorderable.tsx

+     * @param newIndex the new index of the element or set of elements
+     * @param length the number of contiguous elements that were moved
+     */
+    onReordered: (oldIndex: number, newIndex: number, length: number) => void;


onReorderEnd might be more intuitive.

Yeah, I couldn't find great precedent for this in the codebase. Slider does onChange and onRelease, and DragSelectable coincidentally now has onSelection and onSelectionEnd as of a recent commit in this PR. Regardless, do you think onReordering still makes sense if we change onReordered to onReorderEnd?

well, wouldn't onReorder work? You have onReordering while it's going then onReorder once it's finished. That kind of matches the other method names

adidahiya · 2017-04-13T05:06:19Z

packages/table/src/interactions/reorderable.tsx

+     * @param newIndex the new index of the element or set of elements
+     * @param length the number of contiguous elements that were moved
+     */
+    onReordering: (oldIndex: number, newIndex: number, length: number) => void;


what is this useful for?

Updating the position of the vertical and horizontal guides as you drag. See table.tsx.

blueprint-bot · 2017-04-13T09:53:22Z

Make headers' isReorderable prop @internal

Preview: documentation | landing | table
Coverage: core | datetime

themadcreator · 2017-04-13T15:17:55Z

@cmslewis if I were you, I'd be itching to merge this but I do think that perf regression is pretty bad. Also, it's probably just a matter of checking the stack of shouldComponentUpdates and making sure we're not creating new function instances for props, etc.

Drop some console logs in the render methods and compare before after your changes. If you can't find anything obvious with a concerted effort, we can move on, but I expect users who upgrade to this will not be happy.

Edit: Upon further investigation, I've noticed the same perf issue in @gscshoyru's auto-resize PR. So either you both broke it in the same way, or some other change caused the issue. Would still like someone to look into it though less pressure to do it in this PR.

themadcreator · 2017-04-13T15:21:23Z

@cmslewis re: RowHeader vs. ColumnHeader, I also noticed this and other PR reviewers have suggested it as well!

I think I didn't originally do it because there were so many places where we use x/width vs. y/height sprinkled around that it seemed a little annoying to combine. But, If you can reduce those class files by moving at least a few methods into an abstract class, go for it! (not necessary for this PR obvs)

adidahiya · 2017-04-13T16:08:02Z

Yeah I saw the perf issues on @gscshoyru's PR too. Are you sure it's not a difference caused by using production vs. dev builds of React?

Either way, try using why-did-you-update.

giladgray · 2017-04-13T18:09:20Z

approving since the code change looks legit, but we def need some serious perf work ASAP.

cmslewis · 2017-04-13T18:15:15Z

@themadcreator @adidahiya @giladgray would love to look into perf stuff ASAP. From chatting offline, sounds like our plan is to merge this PR and then address perf immediately in another one. The next Blueprint release would block on the followup PR merging. Sound good?

cmslewis · 2017-04-13T18:17:25Z

Also @giladgray FYI I just pushed a fix for a minor visual regression (hiding selected regions while resizing) and added tests to verify everything works there as expected.

blueprint-bot · 2017-04-13T18:20:31Z

Re-add isGuidesShowing behavior + unit tests

Preview: documentation | landing | table
Coverage: core | datetime

themadcreator

Cool, let's do this! What a sweet feature

themadcreator · 2017-04-13T19:44:43Z

packages/table/test/harness.ts

@@ -56,6 +56,10 @@ export class ElementHarness {
        this.element = element;
    }

+    public exists() {
+        return this.element != null;
+    }


adidahiya · 2017-04-13T19:45:47Z

cmslewis added 30 commits March 30, 2017 14:27

Added function description for isAdditive

e068c8b

Initial work on reorderable.tsx

77bad77

Implement logic for determining new index on drag

cbeee34

Have ColumnHeader extend from IReorderableProps

4d7aab0

Basic column reordering now works

f3e02f9

Temporarily update the format table example to allow reordering

a89ab36

Should undo before merging

Show a vertical guide when dragging with the mouse down

036b878

Basic row reordering now works

a60e886

(Maybe) fix box-shadow SCSS rules for vert/horz guides

5e2c0b8

Fix build (use @bluprintjs/core instead of absolute path)

9e7663a

Allow dragging of multiple columns/rows at once

f9dc1d7

[Temporary] Change format-table example to support multi-column editing

b501590

Merge branch 'master' into cl/table-reorder-cols-rows

c1bbb3a

Decomp toAbsoluteNewIndex in table.tsx

1903c1d

Merge branch 'master' into cl/table-reorder-cols-rows

597265b

Add onSelectedRegionMouseDown to DragSelectable

e3fcbd3

Initial work getting DragReorderable to play nicely with DragSelectable

6741e64

Add stopImmediatePropagation support to DragEvents

80a62e2

Use it in DragSelectable to prevent DragReorderable from triggering its handleActivate on the same mousedown event.

Can now move contiguous multi-column regions

40d29fb

Add tests for Locator#convertPointTo{Row,Column}{Top,Left}Boundary

bd37527

Add guideIndexToReorderedIndex and reorderedIndexToGuideIndex utils (…

5c9ef7a

…plus tests)

Integrate the new util functions. Much simpler!

d7087c8

Cleanup columns and reorderable code

c1b343a

Make selection + reordering work for rows now

2ca255a

Delete console.logs in selectable.tsx

18755f6

Delete console.log from dragEvents.ts

feb69a0

Fix bug with dragging a multi-column range within itself

3bc1df5

Disable reordering if there is more than one selected region

b56ec76

Improve comments

7e940d5

Add ability to enable/disable reordering, and improve APIs

29c265d

Bugfix: enable reordering right away if selection exists on mount

444b019

Fix tests and increase coverage to 79% again

c69ea24

cmslewis added 2 commits April 12, 2017 18:25

Write more unit tests for reordering from the table level

443da22

Update hasSelectionEnded on componentWillReceiveProps

ae4a69e

giladgray reviewed Apr 13, 2017

View reviewed changes

Respond to Gilad CR feedback

b0f2edd

adidahiya reviewed Apr 13, 2017

View reviewed changes

Make headers' isReorderable prop @internal

f6f9f9a

giladgray approved these changes Apr 13, 2017

View reviewed changes

Re-add isGuidesShowing behavior + unit tests

ed32926

themadcreator approved these changes Apr 13, 2017

View reviewed changes

adidahiya merged commit 2afd5ea into master Apr 13, 2017

adidahiya deleted the cl/table-reorder-cols-rows branch April 13, 2017 19:46

cmslewis removed the Status: work in progress label Apr 13, 2017

tgreenwatts mentioned this pull request Apr 21, 2017

[Table] Investigate perf regression #1025

Closed

[Table] Support for column and row reordering #963

[Table] Support for column and row reordering #963

Conversation

cmslewis commented Apr 7, 2017 • edited Loading

Fixes #213

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

cmslewis commented Apr 12, 2017 • edited Loading

blueprint-bot commented Apr 12, 2017

Bugfix: enable reordering right away if selection exists on mount

blueprint-bot commented Apr 12, 2017

Fix tests and increase coverage to 79% again

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Apr 13, 2017 • edited Loading

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

blueprint-bot commented Apr 13, 2017

Update hasSelectionEnded on componentWillReceiveProps

cmslewis commented Apr 13, 2017 • edited Loading

blueprint-bot commented Apr 13, 2017

Respond to Gilad CR feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Apr 13, 2017 • edited Loading

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

blueprint-bot commented Apr 13, 2017

Make headers' isReorderable prop @internal

themadcreator commented Apr 13, 2017 • edited Loading

themadcreator commented Apr 13, 2017

adidahiya commented Apr 13, 2017

giladgray commented Apr 13, 2017

cmslewis commented Apr 13, 2017

cmslewis commented Apr 13, 2017

blueprint-bot commented Apr 13, 2017

Re-add isGuidesShowing behavior + unit tests

themadcreator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Apr 13, 2017

cmslewis commented Apr 7, 2017 •

edited

Loading

cmslewis commented Apr 12, 2017 •

edited

Loading

cmslewis Apr 13, 2017 •

edited

Loading

cmslewis commented Apr 13, 2017 •

edited

Loading

cmslewis Apr 13, 2017 •

edited

Loading

themadcreator commented Apr 13, 2017 •

edited

Loading