[Table] Edit focused cells on f2 #1729

gscshoyru · 2017-10-17T21:32:44Z

Changes proposed in this pull request:

Editable cells can be edited when focused by pressing f2 -- so one can edit editable tables merely with f2, enter, and the arrow keys -- no mouse required!

This is done by making cells able to be browser-focused, and having them focus themselves whenever, well, they become focused. This also lets the focus stay on the table when you're done editing a cell.

Reviewers should focus on:

There's... a bit of a horrible hack to allow the hotkeys to... pass through to the div beneath. I'm not sure if we should keep this or if there's a better way, or what? We could make some sort of slightly better version of cells having these endpoints, and make it less of a hack, maybe? Or fix hotkeys to find the DOM element underneath using ReactDOM.findDOMNode?

This doesn't work as well with the cell render batching if you're far down and to the right enough -- you can press f2 before the rendering is caught up and be editing the wrong cell for a short period of time before the rendering catches up and snaps it away. I'm... not sure what to do about this.

Screenshot

blueprint-bot · 2017-10-17T21:41:07Z

Edit focused cells on f2

Preview: documentation | table
Coverage: core | datetime

cmslewis

Seems like a reasonable approach overall! A few things.

cmslewis · 2017-10-18T17:16:39Z

packages/table/src/cell/cell.tsx

+    tabIndex?: number;
+
+    /**
+     * Callback to be called when cell is focused and key is pressed down.


Articles or no articles? That is the question. Whether 'tis nobler in (the?) mind to suffer...

You should probably add to this comment whether or not these callbacks are called while the cell is being edited.

They are... but since you're in a text field they're ignored. These are used by the hotkeys -- i.e. this is the hack, passing the props down to the actual dom element.

cmslewis · 2017-10-18T17:16:54Z

packages/table/src/cell/cell.tsx

+    onKeyDown?: React.KeyboardEventHandler<HTMLElement>;
+
+    /**
+     * Callback to be called when cell is focused and key is released.


No articles

cmslewis · 2017-10-18T17:17:03Z

packages/table/src/cell/cell.tsx

+    onKeyUp?: React.KeyboardEventHandler<HTMLElement>;
+
+    /**
+     * A ref handle to capture the outer div of this cell. Used internally.


cmslewis · 2017-10-18T17:20:48Z

packages/table/src/cell/editableCell.tsx


 import * as Classes from "../common/classes";
 import { Draggable } from "../interactions/draggable";
 import { Cell, ICellProps } from "./cell";

 export interface IEditableCellProps extends ICellProps {
    /**
+     * Whether or not the given cell is the current active/focused cell.


Do we need the or not?

https://english.stackexchange.com/questions/3382/whether-or-not-vs-whether

cmslewis · 2017-10-18T17:21:35Z

packages/table/src/cell/editableCell.tsx

+            <Hotkeys>
+                <Hotkey
+                    key="edit-cell"
+                    label="Edit the currely focused cell"


typo currely => currently

cmslewis · 2017-10-18T17:25:12Z

packages/table/src/cell/editableCell.tsx

+        }
+    }
+
+    private handleCellRef = (cellRef: HTMLElement) => {


Our convention for ref handlers is:

private cellRef: HTMLElement; // ... private refHandlers = { cell: (ref: HTMLElement) => this.cellRef = ref; };

cmslewis · 2017-10-18T17:27:57Z

packages/table/src/tableBodyCells.tsx

@@ -24,6 +25,11 @@ export interface ITableBodyCellsProps extends IRowIndices, IColumnIndices, IProp
    cellRenderer: ICellRenderer;

    /**
+     * The location of the focused cell, for setting the "focused" property on cells


Better phrasing: "The coordinates of the currently focused cell" or "The currently focused cell"? No need to explain why it's needed IMO. Or, since this is just a codebase-internal component, we can explicitly say "..., for setting the 'isFocused' prop on cells."

Need trailing period.

cmslewis · 2017-10-18T17:30:16Z

packages/table/src/tableBodyCells.tsx

@@ -187,7 +193,8 @@ export class TableBodyCells extends React.Component<ITableBodyCellsProps, {}> {
        const cellLoading = baseCell.props.loading != null ? baseCell.props.loading : loading;

        const style = { ...baseCell.props.style, ...Rect.style(rect) };
-        return React.cloneElement(baseCell, { className, key, loading: cellLoading, style });
+        const isFocused = focusedCell != null && focusedCell.row === rowIndex && focusedCell.col === columnIndex;


Can we add a CellUtils.cellsEqual utility function? (in common/internal/cellUtils + a few quick unit tests). This check seems like something that could pop up again.

Don't we have something like this for regions?

I'm... not sure how cellsEqual or anything of the like would make sense here. I'm comparing a focused coordinates with two numbers passed into the function. If I made a cellsEqual, what would I be passing it as arguments, exactly?

Ah, didn't look closely at the comparison values. You're right.

themadcreator

This seems fine. Proposal for additional hotkey rejiggering:

press enter or f2, get into append edit mode, nothing is selected, cursor at the end
press any key that inserts a char, get into replace edit mode directly

Also, not sure what hackery you were referring to with hotkeys

themadcreator · 2017-10-18T18:42:08Z

packages/table/src/cell/_cell.scss

+
+  &:focus {
+    // disable focus outline on cells; we already have the focus border
+    outline: none;


does this rule affect accessibility? I'm not sure how accessibility features in browsers deal with focus style overrides. maybe it doesn't matter?

This only affects visuals, not interactivity

themadcreator · 2017-10-18T18:43:11Z

packages/table/src/cell/cell.tsx

+    tabIndex?: number;
+
+    /**
+     * Callback to be called when cell is focused and key is pressed down.


You should probably add to this comment whether or not these callbacks are called while the cell is being edited.

themadcreator · 2017-10-18T18:44:56Z

packages/table/src/cell/cell.tsx

@@ -132,7 +165,15 @@ export class Cell extends React.Component<ICellProps, {}> {
        const content = <div className={textClasses}>{modifiedChildren}</div>;

        return (


I've started using destructuring assignment in bp-chart for cases like this. e.g.:

{...{ style, tooltip, tabIndex, onKeyDown, onKeyUp }}

themadcreator · 2017-10-18T18:56:34Z

packages/table/src/tableBodyCells.tsx

@@ -187,7 +193,8 @@ export class TableBodyCells extends React.Component<ITableBodyCellsProps, {}> {
        const cellLoading = baseCell.props.loading != null ? baseCell.props.loading : loading;

        const style = { ...baseCell.props.style, ...Rect.style(rect) };
-        return React.cloneElement(baseCell, { className, key, loading: cellLoading, style });
+        const isFocused = focusedCell != null && focusedCell.row === rowIndex && focusedCell.col === columnIndex;


Don't we have something like this for regions?

giladgray · 2017-10-18T22:04:58Z

found a bug! can press f2 repeatedly to put a bunch of cells in editing state:

gscshoyru · 2017-10-19T18:16:36Z

@themadcreator I think we can do the different and additonal hotkeys stuff in a later commit, yes? Unless you all think it'd be better if it happened now, in this commit.

Also the hackery is in Cell.tsx -- hotkeys passes down a tabIndex and onKeyDown/Up props -- which if the child is not a DOM element (such as in this case) they are ignored. So I added them as props to the Cell and pass them along manually.

@giladgray how exactly are you surfacing that?

blueprint-bot · 2017-10-19T18:54:33Z

Add tab/enter nav while editing, address comments

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-10-24T18:29:49Z

packages/table/src/cell/cell.tsx

+    tabIndex?: number;
+
+    /**
+     * Callback to be called when the cell is focused and a key is pressed down.


nit: Callback invoked is more concise

cmslewis · 2017-10-24T18:29:53Z

packages/table/src/cell/cell.tsx

+    onKeyDown?: React.KeyboardEventHandler<HTMLElement>;
+
+    /**
+     * Callback to be called when the cell is focused and a key is released.


cmslewis · 2017-10-24T18:31:05Z

packages/table/src/cell/editableCell.tsx

+    private refHandlers = {
+        cell: (ref: HTMLElement) => {
+            this.cellRef = ref;
+        },


nit: Change this to a one-liner:

cell: (ref: HTMLElement) => this.cellRef = ref,

Prettier does this. Or at least, adds parens if I don't use curlies, which is honestly silly.

cmslewis · 2017-10-24T18:32:12Z

@gscshoyru feel free to address the nits if you want, then merge (or LMK if you want me to merge).

blueprint-bot · 2017-10-25T15:32:24Z

Nits

Preview: documentation | table
Coverage: core | datetime

Edit focused cells on f2

5eb55fe

gscshoyru requested review from cmslewis, giladgray and llorca and removed request for giladgray October 17, 2017 21:32

cmslewis changed the title ~~Edit focused cells on f2~~ [Table] Edit focused cells on f2 Oct 18, 2017

cmslewis suggested changes Oct 18, 2017

View reviewed changes

cmslewis added Domain: accessibility Package: table labels Oct 18, 2017

themadcreator approved these changes Oct 18, 2017

View reviewed changes

Add tab/enter nav while editing, address comments

8fa7b3c

gscshoyru force-pushed the gsc/edit-cell-on-f2 branch from 032ab32 to 8fa7b3c Compare October 19, 2017 18:46

cmslewis approved these changes Oct 24, 2017

View reviewed changes

Nits

2c808b2

gscshoyru merged commit 761d35f into master Oct 25, 2017

cmslewis mentioned this pull request Nov 10, 2017

Prepare Release v1.33.0 #1802

Merged

adidahiya deleted the gsc/edit-cell-on-f2 branch December 20, 2017 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Edit focused cells on f2 #1729

[Table] Edit focused cells on f2 #1729

gscshoyru commented Oct 17, 2017

blueprint-bot commented Oct 17, 2017

cmslewis left a comment

cmslewis Oct 18, 2017

themadcreator Oct 18, 2017

gscshoyru Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

cmslewis Oct 18, 2017

themadcreator Oct 18, 2017

cmslewis Oct 18, 2017

gscshoyru Oct 19, 2017

cmslewis Oct 19, 2017

themadcreator left a comment

themadcreator Oct 18, 2017

llorca Oct 18, 2017

themadcreator Oct 18, 2017

themadcreator Oct 18, 2017

themadcreator Oct 18, 2017

giladgray commented Oct 18, 2017

gscshoyru commented Oct 19, 2017

blueprint-bot commented Oct 19, 2017

cmslewis Oct 24, 2017

cmslewis Oct 24, 2017

cmslewis Oct 24, 2017

gscshoyru Oct 25, 2017

cmslewis commented Oct 24, 2017 •

edited

blueprint-bot commented Oct 25, 2017

		@@ -132,7 +165,15 @@ export class Cell extends React.Component<ICellProps, {}> {
		const content = <div className={textClasses}>{modifiedChildren}</div>;

		return (

Navigation Menu

[Table] Edit focused cells on f2 #1729

[Table] Edit focused cells on f2 #1729

Conversation

gscshoyru commented Oct 17, 2017

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

blueprint-bot commented Oct 17, 2017

Edit focused cells on f2

cmslewis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themadcreator 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

giladgray commented Oct 18, 2017

gscshoyru commented Oct 19, 2017

blueprint-bot commented Oct 19, 2017

Add tab/enter nav while editing, address comments

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

cmslewis commented Oct 24, 2017 • edited

blueprint-bot commented Oct 25, 2017

Nits

cmslewis commented Oct 24, 2017 •

edited