Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jan 12, 2017

Conversation

themadcreator
Copy link
Contributor

@themadcreator themadcreator commented Jan 6, 2017

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.

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.
@blueprint-bot
Copy link

Better handling of mouse events in the table body.

Preview: docs | table
Coverage: core | datetime

@themadcreator
Copy link
Contributor Author

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

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doozy PR but very cool stuff

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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), {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private handleConfirm = (value: string) => {
this.setState({ isEditing: false });
if (this.props.onConfirm) {
this.props.onConfirm(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. add !default so these can be customized
  2. make them based on $pt-z-index-content [- 1] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's !default ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neato


/**
* Since the ContextMenuTarget uses the `onContextMenu` prop instead of event
* attachment API, the prop can be lost. This wrapper helps us maintain context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use @default false syntax

@blueprint-bot
Copy link

Correct import

Preview: docs | table
Coverage: core | datetime

@@ -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.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export interface IEditableCellState {
/**
* Stores the editing state of the cell
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

super(props, context);
this.state = {
isEditing: false,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well okay. It's less code anyway.

ref={this.handleCellRef}
>
<Draggable onDoubleClick={this.handleCellDoubleClick}>
<Cell {...this.props} truncated={false} interactive={interactive}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 neato


private handleCellActivate = (_event: MouseEvent) => {
// Cancel edit of active cell when clicking away
if (!this.state.isEditing && document.activeElement instanceof HTMLElement && document.activeElement.blur) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@giladgray giladgray Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@blueprint-bot
Copy link

PR comments, docs

Preview: docs | table
Coverage: core | datetime

@blueprint-bot
Copy link

Declare state in class

Preview: docs | table
Coverage: core | datetime

@giladgray giladgray merged commit fc0e8d4 into master Jan 12, 2017
@giladgray giladgray deleted the bd/dont-swallow-mouse-events branch January 12, 2017 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants