Better handling of mouse events in the table body. #427

Merged
merged 6 commits into from Jan 12, 2017

Projects

None yet

4 participants

@themadcreator
Contributor
themadcreator commented Jan 6, 2017 edited

Fixes #291, #353

Previously, mouse events in the table body were used for selecting regions. With
the addition of interactive cells such as EditableCell components, we must now
provide full mouse interactivity to parts of the table body.

Previously, two instances of Draggable were contending for mouse events. The
first was waiting for double clicks to focus the EditableCell. The second was
listening for clicks and drags for region selection. Both of these were invoking
preventDefault, which was preventing text selection from working within the
EditableCell even when it was in edit mode.

This change adds new props to Draggable to control the use of preventDefault
and stopPropagation on the mouse events. We also added an interactive prop to
the Cell component, which applies a z-index that brings the cell above the
region layer.

Now, the table body will listen for clicks and drags for selection.
EditableCells will listen for double clicks to start editing. When editing is
begun, the cell become "interactive" and it is moved to a z-index above the
selection regions layer. Also during edit mode, the preventDefault prop is
disabled and stopPropagation is enabled to prevent the table body from messing
with selected regions. This prevents an issue where the user starts dragging a
text selection in an EditableCell and ends up selecting regions.

In addition, we performed some cleanup on the table body:

Consolidated ghost and non-ghost render methods, which were nearly identical.

Moved selection interaction component to top-level of table body instead of
attaching listeners to individual cells. This will reduce the total number of
event listeners attached to DOM elements. However, it did necessitate the
creation of a small wrapper to use the ContextMenuTarget decorator.

@themadcreator themadcreator Better handling of mouse events in the table body.
Previously, mouse events in the table body were used for selecting regions. With
the addition of interactive cells such as `EditableCell` components, we must now
provide full mouse interativity to parts of the table body.

Previously, two instances of `Draggable` were contending for mouse events. The
first was waiting for double clicks to focus the `EditableCell`. The second was
listening for clicks and drags for region selection. Both of these were invoking
`preventDefault`, which was preventing text selection from working withing the
`EditableCell` even when it was in edit mode.

This change adds new props to `Draggable` to control the use of `preventDefault`
and `stopPropagation` on the mouse events. We also added an `iteractive` prop to
the `Cell` component, which applies a z-index that brings the cell above the
region layer.

Now, the table body will listen for clicks and drags for selection.
`EditableCell`s will listen for double clicks to start editing. When editing is
begun, the cell become "interative" and it is moved to a z-index above the
selection regions layer. Also during edit mode, the `preventDefault` prop is
disabled and `stopPropagation` is enabled to prevent the table body from messing
with selected regions. This prevents an issue where the user starts dragging a
text selection in an `EditableCell` and ends up selecting regions.

In addition, we performed some cleanup on the table body:

Consolidated ghost and non-ghost render methods, which were nearly identical.

Moved selection interaction component to top-level of table body instead of
attaching listeners to individual cells. This will reduce the total number of
event listeners attached to DOM elements. However, it did necessitate the
creation of a small wrapper to use the `ContextMenuTarget` decorator.
f7b2e0f
@blueprint-bot
Collaborator

Better handling of mouse events in the table body.

Preview: docs | table
Coverage: core | datetime

@themadcreator
Contributor

Of particular interest will be the "Editable Column Names and Cells" preview example

@giladgray

doozy PR but very cool stuff

packages/table/src/cell/cell.tsx
* An optional native tooltip that is displayed on hover
*/
tooltip?: string;
+
+ /**
+ * If true (the default), the cell contents will be wrapped in a div with
@giladgray
giladgray Jan 10, 2017 Member

could use the @default true syntax supported by the docs?

packages/table/src/cell/cell.tsx
- return (<div className={classes} style={style} title={tooltip}>{content}</div>);
+ const { className, intent, interactive, style, tooltip, truncated } = this.props;
+ const content = truncated ?
+ <div className="bp-table-truncated-text">{this.props.children}</div> : this.props.children;
@giladgray
giladgray Jan 10, 2017 Member

there's also .pt-text-overflow-ellipsis in core styles

packages/table/src/cell/cell.tsx
+ const content = truncated ?
+ <div className="bp-table-truncated-text">{this.props.children}</div> : this.props.children;
+ const classes = classNames(
+ "bp-table-cell", className, Classes.intentClass(intent), {
@giladgray
giladgray Jan 10, 2017 Member

this.props.className should come last when using classNames()

packages/table/src/cell/editableCell.tsx
+ private handleConfirm = (value: string) => {
+ this.setState({ isEditing: false });
+ if (this.props.onConfirm) {
+ this.props.onConfirm(value);
@giladgray
giladgray Jan 10, 2017 Member

core utils has safeInvoke(this.props.onConfirm, value) for this pattern https://github.com/palantir/blueprint/blob/master/packages/core/src/common/utils.ts#L18

+Z-index layers
+*/
+$interactive-cell-z-index: 10;
+$region-layer-z-index: 9;
@giladgray
giladgray Jan 10, 2017 Member
  1. add !default so these can be customized
  2. make them based on $pt-z-index-content [- 1] ?
@themadcreator
themadcreator Jan 10, 2017 Contributor

What's !default ?

@giladgray
giladgray Jan 10, 2017 Member

You can assign to variables if they aren’t already assigned by adding the !default flag to the end of the value. This means that if the variable has already been assigned to, it won’t be re-assigned, but if it doesn’t have a value yet, it will be given one.

+
+/**
+ * Since the ContextMenuTarget uses the `onContextMenu` prop instead of event
+ * attachment API, the prop can be lost. This wrapper helps us maintain context
@giladgray
giladgray Jan 10, 2017 Member

what do you mean "event attachment API?" like document.addEventListener?

@themadcreator
themadcreator Jan 10, 2017 Contributor

Yeah, element.addEventListener("contextmenu", ...) versus onContextMenu

@giladgray
giladgray Jan 10, 2017 Member

but React... like of course it doesn't do that.

@giladgray
giladgray Jan 12, 2017 Member

how about saying "uses prop instead of adding event listeners" (paraphrased) for clarity?

+ preventDefault?: boolean;
+
+ /**
+ * Default false, this prevents the event from propagating up to parent
@giladgray
giladgray Jan 10, 2017 Member

use @default false syntax

themadcreator added some commits Jan 12, 2017
@themadcreator themadcreator PR Comments 584e19d
@themadcreator themadcreator Merging master 64c2591
@themadcreator themadcreator Correct import
f71f9b8
@blueprint-bot
Collaborator

Correct import

Preview: docs | table
Coverage: core | datetime

packages/table/src/cell/cell.tsx
@@ -19,6 +19,15 @@ export interface ICellProps extends IIntentProps, IProps {
style?: React.CSSProperties;
/**
+ * If true, the cell will be rendered above overlay layers to enable mouse
+ * interactions within the cell.
+ *
@giladgray
giladgray Jan 12, 2017 Member

don't need the blank line here (see tooltip below) but if you like it then whatevs

packages/table/src/cell/editableCell.tsx
+export interface IEditableCellState {
+ /**
+ * Stores the editing state of the cell
+ */
@giladgray
giladgray Jan 12, 2017 Member

there's no strong need to document state fields cuz they're only used in the component, and this comment doesn't supply new information given the prop name.

packages/table/src/cell/editableCell.tsx
+ super(props, context);
+ this.state = {
+ isEditing: false,
+ };
@giladgray
giladgray Jan 12, 2017 Member

preferable to do the simple public state = { ... } instead of defining the whole constructor (unless of course you need the constructor but pretty sure you don't here)

@themadcreator
themadcreator Jan 12, 2017 Contributor

but isn't there an issue re-using the same object for every instance? I know state objects aren't supposed to be mutated directly, but setting this in the constructor always creates a new object for the instance just to be safe. Mostly I was just following the pattern i saw elsewhere, is this not the way?

@giladgray
giladgray Jan 12, 2017 Member

the compiler writes a constructor for you and puts all those instance properties inside it. same thing with bound private func = () => {} properties.

from /docs/docs/docs.js (can't link to lines):
image

@themadcreator
themadcreator Jan 12, 2017 Contributor

well okay. It's less code anyway.

- ref={this.handleCellRef}
- >
- <Draggable onDoubleClick={this.handleCellDoubleClick}>
+ <Cell {...this.props} truncated={false} interactive={interactive}>
@giladgray
giladgray Jan 12, 2017 Member

🎉 neato

+
+ private handleCellActivate = (_event: MouseEvent) => {
+ // Cancel edit of active cell when clicking away
+ if (!this.state.isEditing && document.activeElement instanceof HTMLElement && document.activeElement.blur) {
@giladgray
giladgray Jan 12, 2017 Member

boolean coercion in the last clause.
could argue that the instanceof check is enough because MDN

@themadcreator
themadcreator Jan 12, 2017 Contributor

Hmm, i wanted to further verify that the blur method was supported even if the element is an HTMLElement. We did the same thing for the focus method below

@giladgray
giladgray Jan 12, 2017 edited Member

understandable! i'll bet @adidahiya wants you to check !== undefined

}
private handleCellDoubleClick = (_event: MouseEvent) => {
- if (this.cellElement == null) {
+ const cellElement = ReactDOM.findDOMNode(this) as HTMLElement;
@giladgray
giladgray Jan 12, 2017 Member

why did you switch from ref to findDOMNode? i'm undecided on which is preferable.

@themadcreator
themadcreator Jan 12, 2017 Contributor

I switched from rendering a regular <div> to a custom component <Cell>. The ref in that case would be a Cell, but i need the HTMLElement. So this just seems more direct.

@giladgray
giladgray Jan 12, 2017 Member

oh snap yeah. makes perfect sense.

+
+/**
+ * Since the ContextMenuTarget uses the `onContextMenu` prop instead of event
+ * attachment API, the prop can be lost. This wrapper helps us maintain context
@giladgray
giladgray Jan 12, 2017 Member

how about saying "uses prop instead of adding event listeners" (paraphrased) for clarity?

@@ -10,6 +10,7 @@ import * as PureRender from "pure-render-decorator";
import * as React from "react";
import * as ReactDOM from "react-dom";
+import { Utils } from "../common/utils";
@giladgray
giladgray Jan 12, 2017 Member

future issue: this is super awkward usage. why not just export individual functions like we do in other utils files?

@themadcreator
themadcreator Jan 12, 2017 Contributor

That's fair, I guess I've always structured utils as an object. But I see your point. For another PR, though

+
+ /**
+ * This prevents the event from propagating up to parent elements.
+ *
@giladgray
giladgray Jan 12, 2017 Member

calling out unnecessary whitespace again, but it's your call.

packages/table/src/tableBody.tsx
- );
+ const rect = isGhost ? grid.getGhostCellRect(rowIndex, columnIndex) : grid.getCellRect(rowIndex, columnIndex);
+ const style = Object.assign({}, cell.props.style, Rect.style(rect));
+ return React.cloneElement(cell, { style, key } as ICellProps);
@giladgray
giladgray Jan 12, 2017 Member

two clones, one object: Utils.assignClasses and right here. for every cell.

is it possible to merge those into one final clone, given that the first one just changes className?

@themadcreator themadcreator PR comments, docs
938ead5
@blueprint-bot
Collaborator

PR comments, docs

Preview: docs | table
Coverage: core | datetime

@themadcreator themadcreator Declare state in class
26a3e19
@blueprint-bot
Collaborator

Declare state in class

Preview: docs | table
Coverage: core | datetime

@giladgray giladgray merged commit fc0e8d4 into master Jan 12, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/palantir CLA signed via membership in "palantir"
Details
@giladgray giladgray deleted the bd/dont-swallow-mouse-events branch Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment