DetailsList: adding an optional column re-ordering feature by laxmikankanala · Pull Request #4857 · microsoft/fluentui

laxmikankanala · 2018-05-13T07:20:50Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Column Reorder support in DetailsList using drag and drop
Based on the mouse position of the event, column target position will be decided.
There is an optional parameter to specify frozen columns to keep the position of those columns intact.

Pending things:

Need to test this with all different options/types of DetailsList
Need to fix the hit area for drag, that is being impacted by marquee selection
Add CSS changes to show the drag items and drop hints

Focus areas to test

Functionality of Column reorder

msftclas · 2018-05-13T07:21:01Z

All CLA requirements met.

JasonGore · 2018-05-14T23:35:02Z

This would definitely be a minor change since it touches multiple interfaces.

Please add tests for the new DetailsColumn component and consider making test cases for DetailsHeader and/or DetailsList components to exercise the new functionality. This will help ensure nobody breaks the functionality you've added. 😃

JasonGore · 2018-05-14T23:37:22Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.types.ts

+  frozenColumnCount?: number;
+
+  /** Callback to handle the column reorder
+   * draggaedIndex is the source column index, that need to be placed in targetIndex


JasonGore · 2018-05-14T23:38:40Z

.../office-ui-fabric-react/src/components/DetailsList/examples/DetailsList.DragDrop.Example.tsx

-            dragDropEvents={ this._getDragDropEvents() }
-          />
-        </MarqueeSelection>
+        {/* <MarqueeSelection selection={ this._selection }> */ }


Why was MarqueeSelection removed? Is there a reason to leave it the codebase commented out?

Reverted these changes in latest commit, after resolving marquee selection issue

shre-verse · 2018-05-17T08:04:28Z

@amitdh is added to the review. #Closed

shre-verse · 2018-05-17T08:08:14Z

packages/office-ui-fabric-react/src/utilities/dragdrop/interfaces.ts

  onDrop?: (item?: any, event?: DragEvent) => void;
  onDragStart?: (item?: any, itemIndex?: number, selectedItems?: any[], event?: MouseEvent) => void;
  onDragEnd?: (item?: any, event?: DragEvent) => void;
+  onDragOver?: (item?: any, event?: DragEvent) => void;


onDragOver?: (item?: any, event?: DragEvent) => void; [](start = 2, length = 53)

Do we have to expose this in IDragDropEvents interface? #Closed

Yes, as this is the only event that gets triggered while dragging on a cell, and the drop hint status needs to be changed while crossing the 50% of cell width

shre-verse · 2018-05-17T08:11:19Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.scss

+  &.isDropping {
+    background: rgb(82, 79, 79);
+  }
+}


Is this according to the UX? Or is this just a placeholder for now? #Closed

For now, just a placeholder

In reply to: 188871768 [](ancestors = 188871768)

shre-verse · 2018-05-17T08:29:47Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+                    >
+                      { column.ariaLabel }
+                    </label>
+                  ) : null,*/


) : null,*/ [](start = 18, length = 11)

Please remove commented code from this file. #Closed

shre-verse · 2018-05-17T08:30:27Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

-                          onClick={ this._onColumnClick.bind(this, column) }
-                          aria-haspopup={ column.columnActionsMode === ColumnActionsMode.hasDropdown }
-                        >
+                (columnReorderOptions && _isDraggable) && this._renderDropHint(columnIndex),


columnReorderOptions && [](start = 17, length = 23)

Required? #Closed

shre-verse · 2018-05-17T08:31:24Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

              ]
            );
-          })
+          }) }


} [](start = 13, length = 1)

Please fix formating. This should go to next line. #Closed

shre-verse · 2018-05-17T08:37:53Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+    // TODO - Handle CSS changes
+  }
+
+  private _getDragDropColumnEvents(): IDragDropEvents {


private _getDragDropColumnEvents(): IDragDropEvents [](start = 2, length = 51)

Why is this required? We can do whatever we are doing here inside IDragDropOptions. #Closed

shre-verse

🕐

shre-verse · 2018-05-21T14:10:28Z

packages/office-ui-fabric-react/src/utilities/dragdrop/DragDropHelper.tsx

          this._events.on(activeTarget.target.root, 'mouseleave', this._onMouseLeave.bind(this, activeTarget.target));
        }
      }
+      // To prevent marquee selection on draggable targets


To prevent [](start = 9, length = 10)

Or simply -
'Prevent marquee selection on draggable targets' #Closed

shre-verse · 2018-05-21T14:13:44Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsColumn.tsx

+    const { column, columnIndex, parentId } = this.props;
+    const { onRenderColumnHeaderTooltip = this._onRenderColumnHeaderTooltip
+    } = this.props;
+    const isDraggable = this.props.isDraggable;


const isDraggable = this.props.isDraggable; [](start = 4, length = 43)

merge this with 2 above ? #Closed

shre-verse · 2018-05-21T14:21:18Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsColumn.tsx

+
+  private _onDragEnd(item?: any, event?: MouseEvent): void {
+    this.props.setDraggedItemIndex!(-1);
+    event!.preventDefault();


event!.preventDefault(); [](start = 4, length = 24)

Why is this required? #Closed

shre-verse · 2018-05-21T14:27:02Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+  private _updateDroppingState(newValue: boolean, event: DragEvent): void {
+    if ((newValue === false || this._draggedColumnIndex === -1) && event.type !== 'drop') {
+      const newDropHintState = this.state.dropHintsState!.map(state => false);
+      this.setState({ dropHintsState: newDropHintState });


this.setState({ dropHintsState: newDropHintState }); [](start = 6, length = 52)

Pass in a function instead. This way of updating the state is not reliable. #Closed

shre-verse · 2018-05-21T14:30:34Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsColumn.tsx

+  onColumnContextMenu?: (column: IColumn, ev: React.MouseEvent<HTMLElement>) => void;
+  dragDropHelper?: IDragDropHelper | null;
+  isDraggable?: boolean;
+  setDraggedItemIndex?: (itemIndex: number) => void;


setDraggedItemIndex?: (itemIndex: number) => void; [](start = 2, length = 50)

Rather than this, the notification should be simple - like 'onColumnDragStart' with no params. The params (index) can be handled inside the DetailsHeader, where this callback is binded to a function. #Closed

betrue-final-final · 2018-05-21T14:37:27Z

Hey @dzearing and @aditima, can you peeps check this one out?

erichdev

When dragging columns, I'm seeing a big performance hit if the drag lasts more than a ~second (i.e. try dragging for a few seconds before releasing). setState is called as you drag, forcing constant rerendering. Quick testing locally shows adding shouldComponentUpdate to DetailsHeader fixes it -- return true if dropHintsState doesn't exists or if new dropHintsState != current dropHintsState.

laxmikankanala · 2018-05-25T05:51:54Z

@erichdev, the issue of frequent renders has been fixed in the latest commit. Now DetailsHeader will get re-rendered only when there is a change in the drop hints.

shre-verse · 2018-05-25T11:08:23Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

      onRenderColumnHeaderTooltip = this._onRenderColumnHeaderTooltip
    } = this.props;

+    if (!this._dragDropHelper) {


if (!this._dragDropHelper) [](start = 4, length = 26)

if (!this._dragDropHelper && this.props.columenReorderOptions) {
this._dragDropHelper = new DragDropHelper();
} #Closed

shre-verse · 2018-05-25T11:10:36Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+      this._draggedColumnIndex = -1;
+    }
+
+  }


Remove the extra line. #Closed

shre-verse · 2018-05-25T11:11:24Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+  }
+
+  private _onDrop(item?: any, event?: DragEvent): void {
+    if (this._draggedColumnIndex !== -1 && event! instanceof DragEvent) {


this._draggedColumnIndex !== -1 [](start = 8, length = 31)

A better check for 'valid' draggedColumnIndex would be a non-negative check. #Closed

shre-verse · 2018-05-25T11:15:41Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.tsx

+    let indexToUpdate = -1;
+    if (eventXposition <= this._dropHintOriginXValues[currentIndex]) {
+
+      indexToUpdate = currentIndex;


NIT - Extra line. #Closed

betrue-final-final · 2018-06-20T15:24:52Z

Hey @laxmikankanala, it looks like the values are truncated way too early. That's why I rejected the screener test. Can you check that out?

shre-verse · 2018-06-20T16:15:22Z

Hey Ben, I just looked at the screenshots for screener test. Seems that these are expected after the increase in left padding (+5px). This regression was called out during the design iteration as well. @piyush<mailto:Piyush.Belchandan@microsoft.com> @niket<mailto:Jain.Niket@microsoft.com> ~Shreyansh Agrawal

…

________________________________ From: Ben Truelove <notifications@github.com> Sent: Wednesday, June 20, 2018 8:55:00 PM To: OfficeDev/office-ui-fabric-react Cc: Shreyansh Agrawal; Manual Subject: Re: [OfficeDev/office-ui-fabric-react] DetailsList: adding an optional column re-ordering feature (#4857) Hey @laxmikankanala<https://github.com/laxmikankanala>, it looks like the values are truncated way too early. That's why I rejected the screener test. Can you check that out? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#4857 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AkO_B3tXToU6EKAaV06lAB8BzADcJentks5t-mlLgaJpZM4T8sZS>.

betrue-final-final · 2018-06-20T16:30:47Z

I did understand that, but it looks like it's still truncating way too early.

aditima

Signing off. Please have @yiminwu / @ThomasMichon and @betrue-final-final take a look and approve as well.

yiminwu · 2018-06-20T21:02:30Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.scss

+   }
+}
+
+.GripperBarVerticalStyle {


nit: can we change to start with lowcase to match others?

yiminwu · 2018-06-20T21:02:37Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsHeader.scss

    text-overflow: clip;
  }
+
+  &:hover .GripperBarVerticalStyle {


GripperBarVerticalStyle [](start = 11, length = 23)

same here

…c-react into laxmika/colreorder

laxmikankanala · 2018-06-21T12:49:00Z

@betrue-final-final, we have fixed the right padding issue. Can you please review now.

betrue-final-final · 2018-06-21T13:33:51Z

I wasn't quite up then, but I just checked it. It was 5:30AM PST ✌️

…4857)" This reverts commit 14d88cc.

dzearing changed the title ~~Laxmika/colreorder~~ DetailsList: adding an optional column re-ordering feature May 14, 2018

laxmikankanala force-pushed the laxmika/colreorder branch from c5b9bb6 to 61af28d Compare May 14, 2018 08:30

JasonGore reviewed May 14, 2018

View reviewed changes

laxmikankanala requested review from cschleiden, dzearing, joschect and lorejoh12 as code owners May 15, 2018 11:19

laxmikankanala force-pushed the laxmika/colreorder branch from e40bf24 to e5fcc6a Compare May 15, 2018 11:29

shre-verse reviewed May 17, 2018

View reviewed changes

shre-verse suggested changes May 17, 2018

View reviewed changes

shre-verse reviewed May 21, 2018

View reviewed changes

erichdev previously requested changes May 23, 2018

View reviewed changes

shre-verse reviewed May 25, 2018

View reviewed changes

laxmikankanala and others added 8 commits June 20, 2018 06:22

Updated DetailsHeader tests and snapshots with columnReorderOptions

1a756a0

UX changes for dragdrop

fe970d6

Added frozen columns from end

3fa74f0

Performance improvements, drophint css changes

001cd1f

Fixed row items drag issue

5bc464a

Fixed row drag issue

cc91cc4

Updating Dragdrop example snapshot

cb94179

Perf fixes and PR comments

0451931

laxmikankanala force-pushed the laxmika/colreorder branch from e736018 to 0451931 Compare June 20, 2018 13:26

aditima approved these changes Jun 20, 2018

View reviewed changes

yiminwu reviewed Jun 20, 2018

View reviewed changes

yiminwu approved these changes Jun 20, 2018

View reviewed changes

laxmikankanala added 2 commits June 21, 2018 04:50

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

b568a27

…c-react into laxmika/colreorder

Resolving PR comments

3a15dbf

betrue-final-final merged commit 14d88cc into microsoft:master Jun 21, 2018

natalieethell mentioned this pull request Jun 21, 2018

Fix minor accessibility bugs in DetailsList #5278

Merged

cliffkoh added a commit that referenced this pull request Jun 21, 2018

Revert "DetailsList: adding an optional column re-ordering feature (#…

21d176c

…4857)" This reverts commit 14d88cc.

erichdev mentioned this pull request Jun 28, 2018

DragDropHelper: Add support for optional onDragOver prop in IDragDropOptions #5380

Merged

2 tasks

laxmikankanala mentioned this pull request Jul 10, 2018

Details List - Column Drag drop changes #5502

Merged

ThomasMichon mentioned this pull request Aug 2, 2018

Align calculated column widths with cell padding in DetailsList #5769

Merged

KevinTCoughlin mentioned this pull request Nov 16, 2018

DetailsList: Remove drag and drop border animation causing style mutations via mergeStyles #7125

Merged

2 tasks

KevinTCoughlin mentioned this pull request Nov 30, 2018

DetailsList: Horizontal and vertical scrolling when items is empty and column reorder is enabled #6394

Closed

ecraig12345 mentioned this pull request Mar 21, 2019

Use lib: es5 to prevent IE 11 breaks #8414

Merged

2 tasks

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Conversation

laxmikankanala commented May 13, 2018

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

msftclas commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonGore commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse commented May 17, 2018

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse left a comment

Choose a reason for hiding this comment

Uh oh!

shre-verse May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shre-verse May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betrue-final-final commented May 21, 2018

Uh oh!

erichdev left a comment

Choose a reason for hiding this comment

Uh oh!

laxmikankanala commented May 25, 2018

Uh oh!

shre-verse May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

msftclas commented May 13, 2018 •

edited

Loading

JasonGore May 14, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 17, 2018 •

edited

Loading

shre-verse May 21, 2018 •

edited

Loading

shre-verse May 21, 2018 •

edited

Loading

shre-verse May 21, 2018 •

edited

Loading

shre-verse May 21, 2018 •

edited

Loading

shre-verse May 21, 2018 •

edited

Loading

shre-verse May 25, 2018 •

edited

Loading

shre-verse May 25, 2018 •

edited

Loading

shre-verse May 25, 2018 •

edited

Loading

shre-verse May 25, 2018 •

edited

Loading

aditima left a comment •

edited

Loading