-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
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.
Of particular interest will be the "Editable Column Names and Cells" preview example |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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), { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add
!default
so these can be customized - make them based on
$pt-z-index-content [- 1]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's !default
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use @default false
syntax
@@ -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. | |||
* |
There was a problem hiding this comment.
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 | ||
*/ |
There was a problem hiding this comment.
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, | ||
}; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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}> |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. | ||
* |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
?
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 nowprovide full mouse interactivity to parts of the table body.
Previously, two instances of
Draggable
were contending for mouse events. Thefirst was waiting for double clicks to focus the
EditableCell
. The second waslistening for clicks and drags for region selection. Both of these were invoking
preventDefault
, which was preventing text selection from working within theEditableCell
even when it was in edit mode.This change adds new props to
Draggable
to control the use ofpreventDefault
and
stopPropagation
on the mouse events. We also added aninteractive
prop tothe
Cell
component, which applies a z-index that brings the cell above theregion 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 isbegun, the cell become "interactive" and it is moved to a z-index above the
selection regions layer. Also during edit mode, the
preventDefault
prop isdisabled and
stopPropagation
is enabled to prevent the table body from messingwith 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.