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

[Table] scrollToRegion instance method #1496

Merged
merged 30 commits into from
Aug 28, 2017
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f661262
Delete commented code
cmslewis Aug 22, 2017
bd91c72
Add method stub
cmslewis Aug 22, 2017
c4d9f6b
Initial implementation
cmslewis Aug 22, 2017
9e694ab
[TEMPORARY?] Add instance to the window object
cmslewis Aug 22, 2017
70a8bb5
Make auto-scrolling work; fix syncViewportPosition bug
cmslewis Aug 23, 2017
bd1c170
Handling for frozen rows/columns
cmslewis Aug 23, 2017
f65c8f4
Fix scroll-correction logic
cmslewis Aug 23, 2017
0ad4d8a
Add instance method to TableQuadrantStack
cmslewis Aug 24, 2017
33bb63b
Refactor logic into new scrollUtils.ts
cmslewis Aug 24, 2017
b4cf85e
Create new common/internal folder
cmslewis Aug 24, 2017
cc7b908
Fix scroll misalignment bug
cmslewis Aug 24, 2017
fcb8aed
Don't correct if scroll is disabled
cmslewis Aug 24, 2017
2c29b21
Prevent programmatic scrolling when scrolling disabled
cmslewis Aug 24, 2017
a29f832
Remove need for Grid in scrollUtils file
cmslewis Aug 24, 2017
a87242d
Fix off-by-one bug
cmslewis Aug 24, 2017
5cbf3b6
Add scrollTo controls to the table example
cmslewis Aug 24, 2017
ca9be6b
Fix lint
cmslewis Aug 24, 2017
f5080e4
Delete animated param for now
cmslewis Aug 24, 2017
28fcb52
Delete window instance
cmslewis Aug 24, 2017
9d1d9f1
Delete console.logs
cmslewis Aug 24, 2017
5281104
Revert changes RE: prereqStateKeyValue
cmslewis Aug 24, 2017
839c027
Delete unintentional cnewline
cmslewis Aug 24, 2017
89b7b05
Write tests for scrollUtils
cmslewis Aug 24, 2017
da186b5
Fix lint again
cmslewis Aug 25, 2017
dfb5a9c
Write tests in table too
cmslewis Aug 25, 2017
26b4fc8
Added docs, new 'Instance methods' section
cmslewis Aug 25, 2017
70ca679
Update stuff per CR feedback
cmslewis Aug 25, 2017
f21b683
Oops, revert changes to table example
cmslewis Aug 25, 2017
ec3e915
Fix stuff per bdwyer CR
cmslewis Aug 26, 2017
80dfb99
Merge branch 'master' into cl/table-scroll-to-position
cmslewis Aug 28, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions packages/table/preview/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import * as React from "react";
import * as ReactDOM from "react-dom";

import {
Button,
Classes,
FocusStyleManager,
Intent,
Menu,
MenuDivider,
MenuItem,
Expand Down Expand Up @@ -73,6 +75,9 @@ interface IMutableTableState {
numFrozenCols?: number;
numFrozenRows?: number;
numRows?: number;
scrollToColumnIndex?: number;
scrollToRegionType?: RegionCardinality;
scrollToRowIndex?: number;
selectedFocusStyle?: FocusStyle;
showCallbackLogs?: boolean;
showCellsLoading?: boolean;
Expand Down Expand Up @@ -110,6 +115,13 @@ const ROW_COUNTS = [
const FROZEN_COLUMN_COUNTS = [0, 1, 2, 5, 20, 100, 1000];
const FROZEN_ROW_COUNTS = [0, 1, 2, 5, 20, 100, 1000];

const REGION_CARDINALITIES = [
RegionCardinality.CELLS,
RegionCardinality.FULL_ROWS,
RegionCardinality.FULL_COLUMNS,
RegionCardinality.FULL_TABLE,
];

enum CellContent {
EMPTY,
CELL_NAMES,
Expand Down Expand Up @@ -153,6 +165,12 @@ const CELL_CONTENT_GENERATORS = {
class MutableTable extends React.Component<{}, IMutableTableState> {
private store = new DenseGridMutableStore<string>();

private tableInstance: Table;

private refHandlers = {
table: (ref: Table) => this.tableInstance = ref,
};

public constructor(props: any, context?: any) {
super(props, context);

Expand All @@ -176,6 +194,9 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
numFrozenCols: FROZEN_COLUMN_COUNTS[FROZEN_COLUMN_COUNT_DEFAULT_INDEX],
numFrozenRows: FROZEN_ROW_COUNTS[FROZEN_ROW_COUNT_DEFAULT_INDEX],
numRows: ROW_COUNTS[ROW_COUNT_DEFAULT_INDEX],
scrollToColumnIndex: 0,
scrollToRegionType: RegionCardinality.CELLS,
scrollToRowIndex: 0,
selectedFocusStyle: FocusStyle.TAB,
showCallbackLogs: false,
showCellsLoading: false,
Expand Down Expand Up @@ -210,6 +231,7 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
isRowResizable={this.state.enableRowResizing}
loadingOptions={this.getEnabledLoadingOptions()}
numRows={this.state.numRows}
ref={this.refHandlers.table}
renderBodyContextMenu={this.renderBodyContextMenu}
renderRowHeader={this.renderRowHeader}
selectionModes={this.getEnabledSelectionModes()}
Expand Down Expand Up @@ -432,6 +454,8 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
{this.renderSwitch("Callback logs", "showCallbackLogs")}
{this.renderSwitch("Full-table selection", "enableFullTableSelection")}
{this.renderSwitch("Multi-selection", "enableMultiSelection")}
<h6>Scroll to</h6>
{this.renderScrollToSection()}

<h4>Columns</h4>
<h6>Display</h6>
Expand Down Expand Up @@ -479,6 +503,66 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
);
}

private renderScrollToSection() {
const { scrollToRegionType } = this.state;

const scrollToRegionTypeSelectMenu = this.renderSelectMenu(
"Region type",
"scrollToRegionType",
REGION_CARDINALITIES,
this.getRegionCardinalityLabel,
this.handleRegionCardinalityChange,
);
const scrollToRowSelectMenu = this.renderSelectMenu(
"Row",
"scrollToRowIndex",
Utils.times(this.state.numRows, (rowIndex) => rowIndex),
(rowIndex) => `${rowIndex + 1}`,
this.handleNumberStateChange,
);
const scrollToColumnSelectMenu = this.renderSelectMenu(
"Column",
"scrollToColumnIndex",
Utils.times(this.state.numCols, (columnIndex) => columnIndex),
(columnIndex) => this.store.getColumnName(columnIndex),
this.handleNumberStateChange,
);

const ROW_MENU_CARDINALITIES = [RegionCardinality.CELLS, RegionCardinality.FULL_ROWS];
const COLUMN_MENU_CARDINALITIES = [RegionCardinality.CELLS, RegionCardinality.FULL_COLUMNS];

const shouldShowRowSelectMenu = contains(ROW_MENU_CARDINALITIES, scrollToRegionType);
const shouldShowColumnSelectMenu = contains(COLUMN_MENU_CARDINALITIES, scrollToRegionType);

return (
<div>
{scrollToRegionTypeSelectMenu}
<div className="sidebar-indented-group">
{shouldShowRowSelectMenu ? scrollToRowSelectMenu : undefined}
{shouldShowColumnSelectMenu ? scrollToColumnSelectMenu : undefined}
</div>
<Button intent={Intent.PRIMARY} className={Classes.FILL} onClick={this.handleScrollToButtonClick}>
Scroll
</Button>
</div>
);
}

private getRegionCardinalityLabel(cardinality: RegionCardinality) {
switch (cardinality) {
case RegionCardinality.CELLS:
return "Cell";
case RegionCardinality.FULL_ROWS:
return "Row";
case RegionCardinality.FULL_COLUMNS:
return "Column";
case RegionCardinality.FULL_TABLE:
return "Full table";
default:
return "";
}
}

private renderSwitch(
label: string,
stateKey: keyof IMutableTableState,
Expand Down Expand Up @@ -683,6 +767,30 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
this.store.setColumnName(columnIndex, value);
}

private handleScrollToButtonClick = () => {
const { scrollToRowIndex, scrollToColumnIndex, scrollToRegionType } = this.state;

let region: IRegion;
switch (scrollToRegionType) {
case RegionCardinality.CELLS:
region = Regions.cell(scrollToRowIndex, scrollToColumnIndex);
break;
case RegionCardinality.FULL_ROWS:
region = Regions.row(scrollToRowIndex);
break;
case RegionCardinality.FULL_COLUMNS:
region = Regions.column(scrollToColumnIndex);
break;
case RegionCardinality.FULL_TABLE:
region = Regions.table();
break;
default:
return;
}

this.tableInstance.scrollToRegion(region);
Copy link
Contributor

Choose a reason for hiding this comment

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

is invoking methods on the table ref the standard way for actions like this? do we have other components/features that do this? is this what users will expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess the resizeRows does this. still, is this the idiomatic blueprint way to invoke actions on react components?

Copy link
Contributor Author

@cmslewis cmslewis Aug 26, 2017

Choose a reason for hiding this comment

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

Yeah, there are a few instance methods around the codebase, each of which operates on a ref. The goal here was to provide a way to briefly control the scroll without forcing the caller to control it forever and always (would be annoying and unnecessary). See:

// Table
// =====

resizeRowsByTallestCell(columnIndices?: number | number[]): void;

// Toast
// =====

clear(): void;
dismiss(key: string): void;
getToasts(): IToastProps[];
show(props: IToastProps): string;
update(key: string, props: IToastProps): void;

// Tree
// ====

getNodeContentElement(nodeId: string | number): HTMLElement | undefined;

}

// State updates
// =============

Expand Down Expand Up @@ -724,6 +832,10 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
return handleNumberChange((value) => this.setState({ [stateKey]: value }));
}

private handleRegionCardinalityChange = (stateKey: keyof IMutableTableState) => {
return handleNumberChange((value) => this.setState({ [stateKey]: value }));
Copy link
Contributor

Choose a reason for hiding this comment

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

man this pattern is so common, i wish react had a shorthand for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. 4 lines above

}

private updateFocusStyleState = () => {
return handleStringChange((value: string) => {
const selectedFocusStyle = value === "tab" ? FocusStyle.TAB : FocusStyle.TAB_OR_CLICK;
Expand Down Expand Up @@ -821,3 +933,7 @@ function getRandomInteger(min: number, max: number) {
// min and max are inclusive
return Math.floor(min + (Math.random() * (max - min + 1)));
}

function contains(arr: any[], value: any) {
return arr.indexOf(value) >= 0;
}
5 changes: 4 additions & 1 deletion packages/table/src/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ export { Grid } from "./grid";
export { Rect, AnyRect } from "./rect";
export { RoundSize } from "./roundSize";
export { Utils } from "./utils";
// NOTE: Errors is not exported in public API

// NOTE: The following are not exported in the public API:
// - Errors
// - internal/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this common/internal/ folder to clearly denote things that won't be exported?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yes nice

67 changes: 67 additions & 0 deletions packages/table/src/common/internal/scrollUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really want to get into the practice of taking utility code out of table.tsx. I'd also like to avoid putting all utility code into one huge Utils file if possible. Thus, here's a new scrollUtils file.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's cool, if you add more do you think it's useful to keep track of an index.ts that re-exports? it's a tradeoff between maintenance of index.ts and simplifying import lines.

Copy link
Contributor Author

@cmslewis cmslewis Aug 26, 2017

Choose a reason for hiding this comment

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

I'd be into that. 👍 Another immediate idea is focusUtils.ts. Happy to introduce a /common/internal/index.ts file when I create that.

* Copyright 2017 Palantir Technologies, Inc. All rights reserved.
* Licensed under the BSD-3 License as modified (the “License”); you may obtain a copy
* of the license at https://github.com/palantir/blueprint/blob/master/LICENSE
* and https://github.com/palantir/blueprint/blob/master/PATENTS
*/

import { IRegion, RegionCardinality, Regions } from "../../regions";

export function getScrollPositionForRegion(
region: IRegion,
currScrollLeft: number,
currScrollTop: number,
getLeftOffset: (columnIndex: number) => number,
getTopOffset: (rowIndex: number) => number,
numFrozenRows: number = 0,
numFrozenColumns: number = 0,
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of params, but this way I don't have to pass a bloated viewportRect or grid object from <Table>. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

seems legit

Copy link
Contributor

Choose a reason for hiding this comment

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

yeeesh. how many args would passing those props around save? this is an internal method and those args are passed by reference, so no perf penalty.

Copy link
Contributor Author

@cmslewis cmslewis Aug 26, 2017

Choose a reason for hiding this comment

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

It would save two args (get{Top,Left}Offset => grid, currScroll{Top,Left} => viewportRect), but the current args are much easier to mock for unit tests.

Also, we don't really need to pass in the current scroll offsets. We could simply have this function return null for scrollLeft or scrollTop if that respective coordinate doesn't need to change, then the caller can substitute the current value for null in its scope. That reduces the params to:

region: IRegion;
getLeftOffset: (columnIndex: number) => number;
getTopOffset: (rowIndex: number) => number;
numFrozenRows: number = 0;
numFrozenColumns: number = 0;

Thoughts? @themadcreator @giladgray

EDIT: Actually, I guess this isn't quite true given the need for clamping of values within the frozen area. Ignore the above.

const cardinality = Regions.getRegionCardinality(region);

let scrollTop = currScrollTop;
let scrollLeft = currScrollLeft;

// if these were max-frozen-index values, we would have added 1 before passing to the get*Offset
// functions, but the counts are already 1-indexed, so we can just pass those.
const frozenColumnsCumulativeWidth = getLeftOffset(numFrozenColumns);
const frozenRowsCumulativeHeight = getTopOffset(numFrozenRows);

switch (cardinality) {
case RegionCardinality.CELLS: {
// scroll to the top-left corner of the block of cells
const topOffset = getTopOffset(region.rows[0]);
const leftOffset = getLeftOffset(region.cols[0]);
scrollTop = getClampedScrollPosition(topOffset, frozenRowsCumulativeHeight);
scrollLeft = getClampedScrollPosition(leftOffset, frozenColumnsCumulativeWidth);
break;
}
case RegionCardinality.FULL_ROWS: {
// scroll to the top of the row block
const topOffset = getTopOffset(region.rows[0]);
scrollTop = getClampedScrollPosition(topOffset, frozenRowsCumulativeHeight);
break;
}
case RegionCardinality.FULL_COLUMNS: {
// scroll to the left side of the column block
const leftOffset = getLeftOffset(region.cols[0]);
scrollLeft = getClampedScrollPosition(leftOffset, frozenColumnsCumulativeWidth);
break;
}
default: {
// if it's a FULL_TABLE region, scroll back to the top-left cell of the table
scrollTop = 0;
scrollLeft = 0;
break;
}
}

return { scrollLeft, scrollTop };
}

/**
* Adjust the scroll position to align content just beyond the frozen region, if necessary.
*/
function getClampedScrollPosition(scrollOffset: number, frozenRegionCumulativeSize: number) {
// if the new scroll offset falls within the frozen region, clamp it to 0
return Math.max(scrollOffset - frozenRegionCumulativeSize, 0);
}
19 changes: 19 additions & 0 deletions packages/table/src/docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,25 @@ components are available in the __@blueprintjs/table__ package.
The top-level component of the table is `Table`. You must at least define the
number of rows (`numRows` prop) as well as a set of `Column` children.

@#### Instance methods

- `resizeRowsByTallestCell(columnIndices?: number | number[]): void` &ndash; Resizes all rows in the
table to the height of the tallest visible cell in the specified columns. If no indices are
provided, defaults to using the tallest visible cell from all columns in view.
- `scrollToRegion(region: IRegion): void` &ndash; Scrolls the table to the target region in a
fashion appropriate to the target region's cardinality:
- `CELLS`: Scroll the top-left cell in the target region to the top-left corner of the viewport.
- `FULL_ROWS`: Scroll the top-most row in the target region to the top of the viewport.
- `FULL_COLUMNS`: Scroll the left-most column in the target region to the left side of the viewport.
- `FULL_TABLE`: Scroll the top-left cell in the table to the top-left corner of the viewport.

If there are active frozen rows and/or columns, the target region will be positioned in the top-left
corner of the non-frozen area (unless the target region itself is in the frozen area).

If the target region is close to the bottom-right corner of the table, this function will simply
scroll the target region as close to the top-left as possible until the bottom-right corner is
reached.

@interface ITableProps

@### Column
Expand Down
11 changes: 11 additions & 0 deletions packages/table/src/quadrants/tableQuadrantStack.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,17 @@ export class TableQuadrantStack extends AbstractComponent<ITableQuadrantStackPro
this.throttledHandleWheel = CoreUtils.throttleReactEventCallback(this.handleWheel, { preventDefault: true });
}

/**
* Scroll the main quadrant to the specified scroll offset, keeping all other quadrants in sync.
*/
public scrollToPosition(scrollLeft: number, scrollTop: number) {
const { scrollContainer } = this.quadrantRefs[QuadrantType.MAIN];
this.wasMainQuadrantScrollChangedFromOtherOnWheelCallback = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

now THAT is a variable name. 💯

// this will trigger the main quadrant's scroll callback below
scrollContainer.scrollLeft = scrollLeft;
scrollContainer.scrollTop = scrollTop;
}

public componentDidMount() {
this.emitRefs();
this.syncQuadrantSizes();
Expand Down
Loading