Remove InteractionMask #2061

amanmahajan7 · 2020-06-23T22:15:26Z

Scrolling profile

Before

After

It is more or less the same.

src/utils/selectedCellUtils.ts

nstepien · 2020-06-26T13:09:24Z

src/DataGrid.tsx

          <div style={{ height: Math.max(rows.length * rowHeight, clientHeight) }} />
-          {getViewportRows()}
+          {viewportWidth > 0 && getViewportRows()}


Why this change? Does something break when we have viewportWidth === 0?

it was causing the same issue where the grid is initially scrolled

nstepien · 2020-06-26T13:12:03Z

src/DataGrid.tsx

+  const [selectedPosition, setSelectedPosition] = useState<SelectCellState | EditCellState>({ idx: -1, rowIdx: -1, mode: 'SELECT' });
+  const [copiedPosition, setCopiedPosition] = useState<Position & { value: unknown } | null>(null);
+  const [draggedOverRowIdx, setDraggedOverRowIdx] = useState<number | undefined>(undefined);


If you want we can move some state/effects into a non-reusable custom hook, so we can keep the logic in smaller units.

nstepien · 2020-06-26T13:13:39Z

src/DataGrid.tsx

@@ -206,6 +215,9 @@ function DataGrid<R, K extends keyof R, SR>({
  const [scrollTop, setScrollTop] = useState(0);
  const [scrollLeft, setScrollLeft] = useState(0);
  const [columnWidths, setColumnWidths] = useState<ReadonlyMap<string, number>>(() => new Map());
+  const [selectedPosition, setSelectedPosition] = useState<SelectCellState | EditCellState>({ idx: -1, rowIdx: -1, mode: 'SELECT' });


Should we use null as the "nothing selected" state instead?

I think this makes sense to stand for defaultSelectionState. In addition, we can skip empty check for this usage.

https://github.com/adazzle/react-data-grid/pull/2061/files#diff-60532bd8286dc6c925b8ea6ee3e2573bR436

I think it is okay to use a new { idx: -1, rowIdx: -1 } for now. Let's revisit once we add hooks and cleanup code

src/DataGrid.tsx

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

src/Cell.tsx

qili26 · 2020-06-28T20:43:21Z

src/Cell.tsx

@@ -72,6 +111,15 @@ function Cell<R, SR>({
        isRowSelected={isRowSelected}
        onRowSelectionChange={onRowSelectionChange}
      />
+      {enableCellDragAndDrop && isSelected && (


I am thinking instead of keeping this being rendered in the cell component, how about rendering the draggable square to a portal? I see this weird render result for the hover effect. The right bottom corner of the square is not displaying outside of the cell. (MacOS with Chrome)

src/DataGrid.tsx

qili26 · 2020-06-28T21:00:21Z

src/DataGrid.tsx

@@ -129,8 +145,6 @@ export interface DataGridProps<R, K extends keyof R, SR = unknown> {
   */
  /** Toggles whether filters row is displayed or not */
  enableFilters?: boolean;
-  /** Toggles whether cells should be autofocused */
-  enableCellAutoFocus?: boolean;


What was this for at first? Making (0,0) cell selected upon mounting the grid?

Correct. I think we should not steal the focus and let the user select the first cell using grid.selectCell

src/DataGrid.tsx

qili26 · 2020-06-30T21:54:29Z

src/DataGrid.tsx

@@ -670,6 +673,7 @@ function DataGrid<R, K extends keyof R, SR>({
          rowClass={rowClass}
          top={rowIdx * rowHeight + totalHeaderHeight}
          setDraggedOverRowIdx={isDragging ? setDraggedOverRowIdx : undefined}
+          dragHandle={selectedPosition.rowIdx === rowIdx ? getDragHandle() : undefined}


This looks weird combining this condition with the checks inside the getDragHandle function at ln#635. I think it will be better for maintenance to either add a checking outside like this

dragHandle={displayDragHandle() ? getDragHandle() : undefined} // remove the checks from getDragHandle

or completely move all conditions inside getDragHandle like this

function getDragHandle() { if (selectedPosition.rowIdx !== rowIdx || !enableCellDragAndDrop || !isCellEditable(selectedPosition) || selectedPosition.mode === 'EDIT') return null; ...

I think it is fine. We need to add dragHandle only on the row that has the selected position. I will have to change the signature otherwise getDragHandle(rowIdx). I am happy to revisit after code cleanup

I agree with Qi. We can also do return instead of return null in getDragHandle.

amanmahajan7 · 2020-07-01T16:22:37Z

@nstepien is it worth adding a new Viewport component? Currently we render header and summary rows on each cell navigation. Header and SummaryRow are memoized so I am not sure if there will be any performance gain. It however does extract Viewport logic in a separate component which may be easier to maintain. Thoughts?

src/DataGrid.tsx

nstepien · 2020-07-03T16:16:51Z

src/DataGrid.tsx

+  });
+
+  useEffect(() => {
+    latestDraggedOverRowIdx.current = draggedOverRowIdx;


Instead of doing this, let's have a setter to wrap both setDraggedOverRowIdx and latestDraggedOverRowIdx.

We are using setDraggedOverRowIdx={isDragging ? setDraggedOverRowIdx : undefined}. When the drag starts we rerender all the rows but on subsequent render we only render the row that fires onMouseEnter. If we add a setter then all the rows will be rerendered during dragging, We can use useCallback to prevent that. What do you prefer?

I'd prefer useCallback over useEffect for this. I feel like useEffect could cause rare timing issues.

nstepien · 2020-07-03T16:39:50Z

src/DataGrid.tsx

+    setDraggedOverRowIdx(undefined);
+  }
+
+  function handleMouseDown() {


Maybe we should check that we initiate dragging with the primary mouse button, so we can at least avoid a re-render.

nstepien · 2020-07-03T16:53:16Z

src/DataGrid.tsx

@@ -371,6 +548,88 @@ function DataGrid<R, K extends keyof R, SR>({
    }
  }

+  function isDraggedOver(currentRowIdx: number): boolean {


If you want, we can reduce the size of the file by moving utils like this one into a separate module, we'd just need to pass more parameters.

src/DataGrid.tsx

nstepien · 2020-07-03T17:00:57Z

src/DataGrid.tsx

@@ -670,6 +673,7 @@ function DataGrid<R, K extends keyof R, SR>({
          rowClass={rowClass}
          top={rowIdx * rowHeight + totalHeaderHeight}
          setDraggedOverRowIdx={isDragging ? setDraggedOverRowIdx : undefined}
+          dragHandle={selectedPosition.rowIdx === rowIdx ? getDragHandle() : undefined}


I agree with Qi. We can also do return instead of return null in getDragHandle.

src/DataGrid.tsx

src/Cell.tsx

nstepien · 2020-07-03T17:26:17Z

src/Cell.tsx

      'rdg-cell-frozen': column.frozen,
-      'rdg-cell-frozen-last': column.idx === lastFrozenColumnIndex
+      'rdg-cell-frozen-last': column.idx === lastFrozenColumnIndex,
+      'rdg-cell-selected': isSelected,
+      'rdg-cell-copied': isCopied,
+      'rdg-cell-dragged-over': isDraggedOver


I wonder if we should set all these classes and event handlers at the row level, that way we can pass fewer props down to the Cell. Though that might require some useCallback 🤔

We probably can. Let's revisit in the next PR

src/DataGrid.tsx

Select cell only on update

…cal range

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

src/Cell.tsx

src/common/types.ts

qili26 · 2020-07-07T16:44:46Z

src/common/types.ts

+  onCommitCancel: () => void;
+}
+
+export type SelectedCellProps = {


This type feels strange, shouldn't we extract the same fields and only differentiate the diffs?

interface SelectedCellPropsBase { idx: number; onKeyDown: (event: React.KeyboardEvent<HTMLDivElement>) => void; } interface SelectedCellPropsEdit extends SelectedCellPropsBase { mode: 'EDIT'; editorContainerProps: SharedEditorContainerProps; } interface SelectedCellPropsSelect extends SelectedCellPropsBase { mode: 'SELECT'; dragHandleProps?: Pick<React.HTMLAttributes<HTMLDivElement>, 'onMouseDown' | 'onDoubleClick'>; } export type SelectedCellProps = SelectedCellPropsEdit | SelectedCellPropsSelect;

qili26 · 2020-07-07T16:48:33Z

src/Cell.tsx

+          onRowSelectionChange={onRowSelectionChange}
+        />
+        {selectedCellProps?.dragHandleProps && (
+          <div className="rdg-cell-drag-handle" {...selectedCellProps.dragHandleProps} />


TBH, I don't know if is a good UI. The drag handle looks weird to me when it is expending inside.

It is difficult to style it from what I can tell, @nstepien is there a way to make it look like the old drag handle?

We can't make it overflow without degrading performance, we could use a portal but then it wouldn't be in perfect sync when scrolling.

qili26 · 2020-07-07T16:51:11Z

src/DataGrid.tsx

+import { CellNavigationMode, SortDirection, UpdateActions } from './common/enums';
+
+interface SelectCellState extends Position {
+  mode: 'SELECT';


Combining with my type comment here. https://github.com/adazzle/react-data-grid/pull/2061/files#diff-6859ab7932058080e7f916f3bd133a80R108

I think we can do the following

interface SelectCellMode { mode: 'SELECT'; } interface SelectCellState extends Position, SelectCellMode {};

Further, the SelectCellMode can be reused in the types.ts for the props definition

I think it is fine as we only have 2 places where we define mode. It feels like we may be adding too many interfaces with this approach.

src/DataGrid.tsx

qili26 · 2020-07-07T17:58:38Z

src/DataGrid.tsx

+    return isDraggedOver ? selectedPosition.idx : undefined;
+  }
+
+  function getSelectedCellProps(rowIdx: number): SelectedCellProps | undefined {


This function is called on each render, and then passed the return result into the Row. I am wondering if we shall add memorization to the output to prevent unnecessary Row re-render.

It is only passed to the row containing the selected cell so we only rerender 2 rows max. The performance is good and it is tricky to memoize this function. I think it is fine for now

src/DataGrid.tsx

qili26

🚢

amanmahajan7 · 2020-07-09T02:48:33Z

I faced the focus issue again while profiling. I think React is not able to register the change in the focused element. I will try adding keydown on the root element

amanmahajan7 · 2020-07-13T21:08:39Z

src/DataGrid.tsx

+          return shiftKey ? { idx: columns.length - 1, rowIdx: rows.length - 1 } : { idx: 0, rowIdx: 0 };
+        }
+        return { idx: idx + (shiftKey ? -1 : 1), rowIdx };
+      case 'Home':


https://www.w3.org/TR/wai-aria-practices/#keyboard-interaction-for-data-grids

nstepien · 2020-07-14T14:10:28Z

style/cell.less

+  border-left: 1px dashed black;
+  border-right: 1px dashed black;
+  padding-left: 7px;


Should we remove these three lines?
I don't know what the padding does either.

Each cell only has the right and the bottom border and we need to set left and right border when dragged over. This shifts the cell content so I had to change the left-padding to 8px- 1px (border-left-width)

react-data-grid/style/cell.less

Line 5 in 6ffb06a

padding: 0 8px;

The last dragged over cell should have a bottom border but we will fix it later

nstepien · 2020-07-14T14:53:31Z

src/DataGrid.tsx

+    prevSelectedPosition.current = selectedPosition;
+    scrollToCell(selectedPosition);
+    if (document.activeElement !== rowsContainerRef.current) {
+      rowsContainerRef.current!.focus({ preventScroll: true });


TIL preventScroll
https://developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/focus
Looks like it may not be supported by Safari, dunno if that'll be an issue or not going forward.

Sticky css is not working well with Safari either. Not sure if we discussed that as well. If we had, perhaps this wouldn't be an issue.

I implemented focus in a different way and { preventScroll } is no longer needed as the focused element never needs to be scrolled. I think in the next PR we can push focusing logic to the next level

Scroll the the last selected cell on focus

Tabbing in an out of the grid can be further improved

We can set tabIndex=-1 on the checkbox selector and pass a flag isCellActive = isSelected && isFocused flag to formatters. Then the formatters can choose to focus the checkbox when the cell is active. This way keyboard selection would work. Same thing can be done for treeview example.

nstepien · 2020-07-14T15:21:36Z

src/DataGrid.tsx

+              height: Math.max(rows.length * rowHeight, clientHeight),
+              outline: 0,
+              position: 'relative',
+              top: -totalHeaderHeight // Prevents scrolling when grid receives focus via keyboard


Let's move these to core.less

Initial commit

1b998a3

amanmahajan7 self-assigned this Jun 23, 2020

amanmahajan7 and others added 3 commits June 23, 2020 17:19

Copy Tab logic

2eaa5a0

Initial cell drag implementation

b085271

Initial cell editing implementation

1bc4eca

nstepien requested changes Jun 26, 2020

View reviewed changes

Mahajan and others added 6 commits June 26, 2020 12:58

Move editorContainer to the DataGrid component

54de1df

Update src/utils/selectedCellUtils.ts

85d95a2

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

Cleanup

66b0dfc

Remove masks components

526fa66

Cancel copying

0600a9f

Remove edit check

a596653

qili26 reviewed Jun 28, 2020

View reviewed changes

Mahajan and others added 10 commits June 28, 2020 20:55

Cleanup

888dd59

Address comments

362414e

Move DragHandle to the parent DataGrid component

36d0ce9

Do not paste on the copied cell

ff7d61e

Remove unnecessary class

34f11c1

Fix copy/dragged cell styles

c024ae7

Address dragging issues

f9b9151

Pass down dragHandle component

5e355ed

Fix styles

c4b6ad7

Remove unused function

787ab44

qili26 reviewed Jun 30, 2020

View reviewed changes

Mahajan added 4 commits June 30, 2020 21:00

Move getNextPosition to selectedCellUtils

9404987

Use ref to get the latest draggedOverRowIdx

1b8c3a5

Revert EventBus changes

a9a2613

Fix type errors

336c793

amanmahajan7 marked this pull request as ready for review July 1, 2020 02:40

nstepien requested changes Jul 3, 2020

View reviewed changes

Mahajan and others added 4 commits July 6, 2020 10:57

Add selectedCellProps props

0504d29

Select cell only on update

Remove isMouted check

3cc29bd

Add the row containing the selected cell if not included in the verti…

de4248a

…cal range

Update src/DataGrid.tsx

3acd681

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

amanmahajan7 mentioned this pull request Jul 6, 2020

Remove enableCellAutoFocus prop #2073

Merged

Mahajan added 2 commits July 7, 2020 07:22

Merge branch 'canary' into am-remove-masks

1c53773

Address comments

689c727

qili26 reviewed Jul 7, 2020

View reviewed changes

src/DataGrid.tsx Show resolved Hide resolved

Mahajan added 5 commits July 8, 2020 10:34

Set focus in useLayoutEffect and set tabIndex to -1

a4beb50

setFocus -> shouldFocus

86ca94b

Address comments

ba7d7e3

Cleanup

5a5b9fe

use event.buttons

a4ecddf

qili26 previously approved these changes Jul 8, 2020

View reviewed changes

amanmahajan7 dismissed qili26’s stale review via a4ecddf July 9, 2020 01:43

Mahajan added 2 commits July 13, 2020 12:48

Better focus handling

eda56d0

Remove comments

9011686

amanmahajan7 commented Jul 13, 2020

View reviewed changes

Check valid selection

421d265

nstepien reviewed Jul 14, 2020

View reviewed changes

Mahajan added 3 commits July 14, 2020 18:30

Even better focus implementation

6ffb06a

Cleanup handleKeyDown usage

4eb1100

Remove drag cell borders

c8cb194

nstepien approved these changes Jul 15, 2020

View reviewed changes

amanmahajan7 merged commit f94aa4e into canary Jul 15, 2020

amanmahajan7 deleted the am-remove-masks branch July 15, 2020 17:47

jhoneybee mentioned this pull request Jul 17, 2020

bug: when the scroll bar appears, the position of the edit box is displayed incorrectly. #2087

Closed

Remove InteractionMask #2061

Remove InteractionMask #2061

Conversation

amanmahajan7 commented Jun 23, 2020 • edited

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

qili26 Jun 28, 2020 • edited

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

amanmahajan7 commented Jul 1, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qili26 left a comment

Choose a reason for hiding this comment

amanmahajan7 commented Jul 9, 2020

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

qili26 Jul 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 commented Jun 23, 2020 •

edited

qili26 Jun 28, 2020 •

edited

qili26 Jul 14, 2020 •

edited