Editor2 #2102

amanmahajan7 · 2020-07-24T21:44:53Z

New API

column.editor2?: React.ComponentType<Editor2Props<TRow, TSummaryRow>>;
column.editorOptions?: {
    /** Default: true for editor1 and false for editor2 */
    createPortal?: boolean;
    /** Default: false */
    editOnClick?: boolean;
    /** Prevent default to cancel editing */
    onCellKeyDown?: (event: React.KeyboardEvent<HTMLDivElement>) => void;
};

export interface Editor2Props<TRow, TSummaryRow = any> {
  row: Readonly<TRow>;
  rowIdx: number;
  column: Readonly<CalculatedColumn<TRow, TSummaryRow>>;
  rowHeight: number;
  onRowChange: (row: Readonly<TRow>, commitChanges?: boolean) => void;
  onClose: (commitChanges?: boolean) => void;
  top: number;
  left: number;
  editorPortalTarget: Element;
}

How to use

1) Use `column.editor2`

   {
      key: 'country',
      name: 'Country',
      width: 180,
      editor2: p => (
        <SelectEditor
          value={p.row.country}
          onChange={value => p.onRowChange({ ...p.row, country: value })}
          options={countries.map(c => ({ value: c, label: c }))}
          rowHeight={p.rowHeight}
          menuPortalTarget={p.editorPortalTarget}
        />
      ),
      resizable: true,
      sortable: true
    },
    {
      key: 'contact',
      name: 'Contact',
      width: 160,
      editor2: p => (
        <TextEditor
          value={p.row.contact}
          onChange={value => p.onRowChange({ ...p.row, contact: value })}
          rowHeight={p.rowHeight}
        />
      ),
      resizable: true,
      sortable: true
    }

2) Handle `onRowsChange`

onRowsChange={updatedRows => setRows(updatedRows )}

DataGrid now keeps the updated row in a local state so it is easy to handle commit, cancel and validations (future).

amanmahajan7 · 2020-07-27T22:23:49Z

src/DataGrid.tsx

+      if (isUsingEditor2) {
+        if (key === 'Delete' || key === 'Backspace') {
+          setEditorValue('');
+        } else if (key === 'Enter' || key === 'F2') {


https://www.w3.org/TR/wai-aria-practices/#gridNav_inside

amanmahajan7 · 2020-07-27T22:25:48Z

src/DataGrid.tsx

@@ -504,8 +548,10 @@ function DataGrid<R, K extends keyof R, SR>({

  function selectCell(position: Position, enableEditor = false): void {
    if (!isCellWithinBounds(position)) return;
+    handleCommit2();


I think it is better to have some control over when the value is committed. This way we can add validation in the future. Using an effect to commit the value is probably not the best way
https://github.com/adazzle/react-data-grid/blob/c/src/editors/EditorContainer.tsx#L70

amanmahajan7 · 2020-07-27T22:27:47Z

src/DataGrid.tsx


-    if (canOpenEditor && (key === 'Enter' || isActivatedByUser)) {
+    const isActivatedByUser = isUsingEditor2
+      ? defaultCellInput(event) && column.editor2Props?.onCellInput?.(event) !== false


I think we should open the editor on Enter, F2, or on any other alpha numeric key. If the users need to restrict it then they can use onCellInput . This is also the recommended way
https://www.w3.org/TR/wai-aria-practices/#gridNav_inside

amanmahajan7 · 2020-07-27T22:28:58Z

src/types.ts

+  editor2Props?: {
+    createPortal?: boolean;
+    editOnSingleClick?: boolean;
+    commitOnOutsideClick?: boolean;


This needs to be implemented.

Perhaps add a TODO comment?

amanmahajan7 · 2020-07-27T22:30:07Z

stories/demos/TreeView.tsx

        }
+        // editor2Props: {


I think we can handle it differently using https://www.w3.org/TR/wai-aria-practices/#gridNav_focus

amanmahajan7 · 2020-07-27T22:30:58Z

stories/demos/components/Editors/TextEditor.tsx

+export function TextEditor<R>({ value, onChange, rowHeight }: Editor2Props<string, R>) {
+  const inputRef = useRef<HTMLInputElement>(null);
+
+  useLayoutEffect(() => {


We no longer have the bug where the input value is initially selected and typing another key deletes it

amanmahajan7 · 2020-07-27T22:31:40Z

stories/demos/components/Editors/SelectEditor.tsx

+        })
+      }}
+      onKeyDown={event => {
+        if (['ArrowDown', 'ArrowUp', 'Enter'].includes(event.key) && selectRef.current!.getProp('menuIsOpen')) {


I think this is the best way to cancel keyboard navigation IMO

qili26 · 2020-07-29T04:19:40Z

src/DataGrid.tsx


-    if (canOpenEditor && (key === 'Enter' || isActivatedByUser)) {
+    if (selectedPosition.mode === 'EDIT') {
+      if (isUsingEditor2 && (key === 'Enter' || key === 'F2')) {


What happens if we have such editors that has 2 or more input / textarea?

I think it is not possible to have one solution for every type of editor and we need some way to to tweak the default behavior. If the editor do not need to commit changes on Enter then it should handle the keydown event and stop propagation. The question is what default behavior we want to use? Right now we do the following

Open editor on Enter, F2 and alphanumeric keys

Cancel changes on Escape

Commit changes on Enter and F2

Commit changes when another cell is clicked

Commit changes when header or summary row is clicked

Any suggestions?

I think for the Enter case, we shall leave a prop for the user to decide if we want to use Enter to close the the editor.
It is a common use case that people might want to put a save button with 2 inputs for example, Enter would mean something else for them. As for F2, maybe we should keep like excel to only open the cell rather than closing it. Esc is pretty straightforward, just close without saving / commit.

qili26 · 2020-07-30T04:49:37Z

src/Cell.tsx

+            rowIdx={rowIdx}
+            row={row}
+            column={column}
+            left={gridLeft}


Should we move left / top before ln#80, so that we can retain the ability for overriding the position to make it more flexible?

Editor can choose not to use left, right props. The only way to override them is in the editor itself. We control editor2Props and not the user

src/DataGrid.tsx

qili26 · 2020-07-30T04:59:48Z

src/types.ts

+  editor2Props?: {
+    createPortal?: boolean;
+    editOnSingleClick?: boolean;
+    commitOnOutsideClick?: boolean;


Perhaps add a TODO comment?

src/utils/keyboardUtils.ts

nstepien · 2020-07-30T13:40:10Z

src/DataGrid.tsx

+    onRowsUpdate?.({
+      cellKey,
+      fromRow: rowIdx,
+      toRow: rowIdx,
+      // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+      updated: { [cellKey]: editorValue } as never,
+      action: UpdateActions.CELL_UPDATE
+    });


I'd like to rethink this someday, maybe something like

// single value update onDataUpdate({ type: ENUM.UPDATE_TYPE, value: unknown, // is it possible to type this? probably not, I think each column should have its own handler row, rowIdx, column }) // multi value update onDataUpdate({ type: ENUM.MULTI_UPDATE_TYPE, updates: [{ value, row, rowIdx, column }] })

An object for updated doesn't make sense to me, going forward we should aim to make rows completely generic.

nstepien · 2020-07-30T13:53:01Z

src/DataGrid.tsx

@@ -223,6 +223,7 @@ function DataGrid<R, K extends keyof R, SR>({
  const [copiedPosition, setCopiedPosition] = useState<Position & { value: unknown } | null>(null);
  const [isDragging, setDragging] = useState(false);
  const [draggedOverRowIdx, setOverRowIdx] = useState<number | undefined>(undefined);
+  const [editorValue, setEditorValue] = useState<unknown>();


Should this state and all the handlers really be in the root component?

We need to control when the value gets committed #2102 (comment) and for that root component needs access to editorValue. Any other way to handle it?

I thought maybe the editor container could have it in its scope

nstepien · 2020-07-30T13:53:45Z

src/DataGrid.tsx

+    if (isCellEditable(selectedPosition) && defaultCellInput(event)) {
+      if (isUsingEditor2) {
+        if (key === 'Delete' || key === 'Backspace') {
+          setEditorValue('');


'' doesn't make sense for non-string columns.

I think we need to define the default behavior and provide a way to override it
#2102 (comment)

src/DataGrid.tsx

nstepien · 2020-07-30T13:58:57Z

src/DataGrid.tsx

+        if (key === 'Delete' || key === 'Backspace') {
+          setEditorValue('');
+        } else if (key === 'Enter' || key === 'F2') {
+          setEditorValue(column.editor2Options?.getInitialValue ? column.editor2Options?.getInitialValue(event, row) : row[column.key as keyof R]);


row[column.key as keyof R] assumes that the row is an object.

nstepien · 2020-07-30T14:49:11Z

src/editors/EditorContainer.tsx

@@ -133,7 +133,7 @@ export default function EditorContainer<R, SR>({
  function onKeyDown(e: KeyboardEvent) {
    if (preventDefaultNavigation(e.key)) {
      e.stopPropagation();
-    } else if (['Enter', 'Tab', 'ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight'].includes(e.key)) {
+    } else if (['Enter', 'F2', 'Tab', 'ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight'].includes(e.key)) {


In Windows' file explorer, if I press F2 twice, it doesn't commit anything, it keeps the editing state.
Same thing in a google spreadsheet.

We can remove it. I was following the accessibility guidelines

nstepien · 2020-07-30T14:55:11Z

src/types.ts

+    getInitialValue?: (event: React.KeyboardEvent<HTMLDivElement>, row: TRow) => unknown;
+    onCellInput?: (event: React.KeyboardEvent<HTMLDivElement>) => void;


What's the difference between these two?

Co-authored-by: Nicolas Stepien <[email protected]>

amanmahajan7 · 2020-07-30T17:40:35Z

src/SummaryRow.tsx

@@ -29,6 +32,7 @@ function SummaryRow<R, SR>({
      aria-rowindex={ariaRowIndex}
      className={`rdg-row rdg-row-${rowIdx % 2 === 0 ? 'even' : 'odd'} rdg-summary-row`}
      style={{ bottom }}
+      onClick={onClick}


I accidentally deleted your comment

Why this change? I don't think we expose SummaryRow?

So we can commit the value when summary row is clicked. We need to decide on when to commit the values

nstepien

Needs a changelog

src/DataGrid.tsx

src/types.ts

nstepien · 2020-07-31T18:25:52Z

src/types.ts

+  editor2Options?: {
+    createPortal?: boolean;
+    singleClickEdit?: boolean;
+    commitOnOutsideClick?: boolean;


Let's remove or comment this out if it won't be implemented in this PR.

It was a quick implementation so I added it

amanmahajan7 · 2020-08-03T16:55:28Z

src/types.ts

+    // editOnDoubleClick?: boolean;
+    // commitOnScroll?: boolean;
+    // commitOnOutsideClick?: boolean;
+    onCellInput?: (event: React.KeyboardEvent<HTMLDivElement>) => void; // Prevent default to cancel editing


Should we rename it to onKeyDown or onCellKeyDown?

It's only used when the editor is closed right? Then onCellKeyDown makes more sense.

amanmahajan7 · 2020-08-03T21:41:49Z

src/DataGrid.tsx

+  }
+
+  function handleSort(columnKey: string, direction: SortDirection) {
+    handleCommit2();


This is still needed as the cell is unmounted before useOutsideClick runs

nstepien · 2020-08-14T12:20:29Z

src/Row.tsx

-      ))}
+      {viewportColumns.map(column => {
+        const isCellSelected = selectedCellProps?.idx === column.idx;
+        if (selectedCellProps?.mode === 'EDIT' && isCellSelected) {


Suggested change

if (selectedCellProps?.mode === 'EDIT' && isCellSelected) {

if (isCellSelected && selectedCellProps!.mode === 'EDIT') {

nstepien · 2020-08-14T12:24:32Z

src/Row.tsx

+            isCellSelected={isCellSelected}
+            isRowSelected={isRowSelected}
+            eventBus={eventBus}
+            dragHandleProps={isCellSelected ? (selectedCellProps as SelectedCellProps)?.dragHandleProps : undefined}


Do we need the type assertion and ?.?

Yeah, TS does not recognize it

if isCellSelected is true, then we can assert that selectedCellProps isn't nullish, so we can add !.

nstepien · 2020-08-14T12:24:41Z

src/Row.tsx

+            isRowSelected={isRowSelected}
+            eventBus={eventBus}
+            dragHandleProps={isCellSelected ? (selectedCellProps as SelectedCellProps)?.dragHandleProps : undefined}
+            onKeyDown={isCellSelected ? selectedCellProps?.onKeyDown : undefined}


Do we need ?.?

Same reason.

Mahajan added 2 commits July 23, 2020 09:32

unsafe_editorProps

8cec9c9

Add editor2

58ace2a

amanmahajan7 changed the title ~~Add unsafe_editorProps~~ Editor2 Jul 27, 2020

amanmahajan7 mentioned this pull request Jul 27, 2020

Editor2 #1953

Closed

3 tasks

amanmahajan7 marked this pull request as ready for review July 27, 2020 22:23

amanmahajan7 commented Jul 27, 2020

View reviewed changes

Mahajan added 5 commits July 27, 2020 17:34

ESlint

d8fcee0

Cleanup

f4a6478

getInitialValue

9721b86

Create countries only once

4959dc8

editor2Props -> editor2Options

c112665

amanmahajan7 self-assigned this Jul 28, 2020

Mahajan added 3 commits July 28, 2020 15:38

Fix types

38f8d41

Tiny style update

93aa39e

Remove editOnSingleClick

6e71e62

qili26 reviewed Jul 30, 2020

View reviewed changes

nstepien reviewed Jul 30, 2020

View reviewed changes

Update src/DataGrid.tsx

c98307a

Co-authored-by: Nicolas Stepien <[email protected]>

adazzle deleted a comment from nstepien Jul 30, 2020

Merge branch 'canary' into am-editorprops

9299b93

amanmahajan7 commented Jul 30, 2020

View reviewed changes

Mahajan added 7 commits July 30, 2020 21:20

Do not commit value on F2 to match the common behavior

a6108f1

Do not commit value on F2 to match the common behavior

7b2a63f

Correctly fix F2 behavior

4ede320

editOnSingleClick -> singleClickEdit

320ba73

New API

bfcba36

Fix portal

30bb22a

Add null check

315b0ad

nstepien requested changes Jul 31, 2020

View reviewed changes

Use editorPortalTarget for SelectEditor

9336cbc

amanmahajan7 and others added 3 commits August 3, 2020 09:15

Merge branch 'canary' into am-editorprops

49e530b

editor2Options - editorOptions

9cd9d05

Update changelog

c6f070c

amanmahajan7 commented Aug 3, 2020

View reviewed changes

Mahajan added 5 commits August 3, 2020 13:20

Add onClose and remove onCommit/onCommitCancel

7ab4b87

Commit editor2 values on outside click

d509999

Cleanup

b761b8b

handleRowsChange -> handleRowChange

adea937

Fix types

0861759

amanmahajan7 commented Aug 3, 2020

View reviewed changes

Mahajan and others added 10 commits August 4, 2020 06:08

More robust useClickOutside

9b2c38b

Use event handlers to set isMenuOpen

4752cc1

Merge branch 'canary' into am-editorprops

549fa54

Merge branch 'canary' into am-editorprops

820a2c3

Merge branch 'canary' into am-editorprops

c483678

Add EditCell component

1404ed6

onRowUpdate -> onRowChange

1b13d38

Revert onSort changes to prevent header rerender on each key

0d6a09d

Pass selectedCell props to the selected cell

4343934

Fix tests

7bbf613

nstepien reviewed Aug 14, 2020

View reviewed changes

Mahajan added 3 commits August 14, 2020 10:19

Address comments

d9e965b

Merge branch 'canary' into am-editorprops

900d3bf

defaultCellInput -> isDefaultCellInput

8db0265

nstepien approved these changes Aug 17, 2020

View reviewed changes

amanmahajan7 merged commit ad19cc4 into canary Aug 17, 2020

amanmahajan7 deleted the am-editorprops branch August 17, 2020 12:50

This was referenced Aug 19, 2020

Unmount cell editor outside viewport #2109

Closed

Cleanup #2128

Merged

amanmahajan7 mentioned this pull request Oct 27, 2020

V7 Editor API (need examples) #2041

Closed

		getInitialValue?: (event: React.KeyboardEvent<HTMLDivElement>, row: TRow) => unknown;
		onCellInput?: (event: React.KeyboardEvent<HTMLDivElement>) => void;

	if (selectedCellProps?.mode === 'EDIT' && isCellSelected) {
	if (isCellSelected && selectedCellProps!.mode === 'EDIT') {

Editor2 #2102

Editor2 #2102

Conversation

amanmahajan7 commented Jul 24, 2020 • edited Loading

New API

How to use

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 Jul 27, 2020 • 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

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 Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

nstepien 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

amanmahajan7 commented Jul 24, 2020 •

edited

Loading

amanmahajan7 Jul 27, 2020 •

edited

Loading

amanmahajan7 Jul 30, 2020 •

edited

Loading

amanmahajan7 Jul 30, 2020 •

edited

Loading