[Table] scrollToRegion instance method #1496

cmslewis · 2017-08-24T20:11:09Z

Fixes #1065, Fixes #1430

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

💎 NEW Table now offers a scrollToRegion(region: IRegion) instance method to programmatically scroll a desired row, column, or cell into view!
- The target region will be positioned in the top/left corner of the viewport.
  - Or if the table is maximally scrolled, then having the target cell simply in view is enough.
- If there are active frozen rows and/or columns:
  - Scrolling to a frozen row will move the table back to scrollTop=0.
  - Scrolling to a frozen column will move the table back to scrollLeft=0.
  - Scrolling to a frozen TOP_LEFT cell will move the table back to (0, 0).
  - Scrolling to a non-frozen row/column will scroll the target region to just beyond the frozen regions, keeping it clearly in view.
🖋 Added documentation for both Table instance methods:
- resizeRowsByTallestCell
- scrollToRegion
Also added some new scrollTo controls on the Table example page.

Followup work (for later)

[Table] Add animated option to scrollToRegion instance method #1501 Add an animated: boolean option to animate the scroll to the new region. // CC @llorca

Reviewers should focus on:

How does this API look? Tried to be as simple and general as possible. Ended up not including an animated option for now, because that'd actually be a lot more work.

Screenshot

cmslewis

Clarifying comments + self-review.

cmslewis · 2017-08-24T20:12:20Z

packages/table/preview/index.tsx

+        );
+
+        // Table: "Scroll to"
+        const scrollToRegionTypeSelectMenu = this.renderSelectMenu(


This whole area is a mess now. Wanted to get a preview up quickly, but would like to clean this up before merging.

cmslewis · 2017-08-24T20:12:52Z

packages/table/preview/index.tsx

    private renderSwitch(
        label: string,
        stateKey: keyof IMutableTableState,
        prereqStateKey?: keyof IMutableTableState,
-        prereqStateKeyValue?: any,
+        prereqStateKeyValues?: any[],


Can undo all these changes; ended up going in a different direction.

cmslewis · 2017-08-24T20:13:10Z

packages/table/src/common/index.ts

+
+// NOTE: The following are not exported in the public API:
+// - Errors
+// - internal/


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

👍 yes nice

cmslewis · 2017-08-24T20:14:09Z

packages/table/src/common/internal/scrollUtils.ts

@@ -0,0 +1,68 @@
+/**


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.

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.

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

cmslewis · 2017-08-24T20:14:37Z

packages/table/src/common/internal/scrollUtils.ts

+    getTopOffset: (rowIndex: number) => number,
+    numFrozenRows: number = 0,
+    numFrozenColumns: number = 0,
+) {


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

seems legit

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.

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.

cmslewis · 2017-08-24T20:15:07Z

packages/table/src/quadrants/tableQuadrantStack.tsx

+        const { scrollContainer } = this.quadrantRefs[QuadrantType.MAIN];
+        this.wasMainQuadrantScrollChangedFromOtherOnWheelCallback = false;
+        // this will trigger the main quadrant's scroll callback below
+


Can delete this new line.

cmslewis · 2017-08-24T20:15:20Z

packages/table/src/quadrants/tableQuadrantStack.tsx

@@ -368,12 +380,17 @@ export class TableQuadrantStack extends AbstractComponent<ITableQuadrantStackPro
    // use the more generic "scroll" event for the main quadrant, which captures both click+dragging
    // on the scrollbar and trackpad/mousewheel gestures
    private handleMainQuadrantScroll = (event: React.UIEvent<HTMLElement>) => {
+        console.log("[TableQuadrantStack] handleMainQuadrantScroll");


Can delete these console.logs.

cmslewis · 2017-08-24T20:16:04Z

packages/table/src/table.tsx

@@ -1030,48 +1081,31 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
        const rowIndexEnd = showFrozenRowsOnly ? numFrozenRows : rowIndices.rowIndexEnd;

        return (
-            // <div


Went ahead and deleted this unrelated commented code. Was a relic from the frozen rows/columns refactor.

cmslewis · 2017-08-24T20:16:58Z

packages/table/src/table.tsx

-            // we need to modify the body element explicitly for the viewport to shift
+            // we need to modify the scroll container explicitly for the viewport to shift. in so
+            // doing, we add the size of the header elements, which are not technically part of the
+            // "grid" concept (the grid only consists of body cells at present).


This was actually a regression that happened during the frozen rows/columns refactor. But all that broke was auto-scrolling as you use arrow keys to move the focus cell around. Still, works again now. 👍

giladgray · 2017-08-24T22:00:13Z

packages/table/src/common/index.ts

+
+// NOTE: The following are not exported in the public API:
+// - Errors
+// - internal/


👍 yes nice

giladgray · 2017-08-24T22:00:48Z

packages/table/src/common/internal/scrollUtils.ts

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


not sure these headings are useful... the keyword export gets the point across.

giladgray · 2017-08-24T22:01:37Z

packages/table/src/common/internal/scrollUtils.ts

+    getTopOffset: (rowIndex: number) => number,
+    numFrozenRows: number = 0,
+    numFrozenColumns: number = 0,
+) {


seems legit

giladgray · 2017-08-24T22:02:15Z

packages/table/src/common/internal/scrollUtils.ts

+        // scroll to the left side of the column block
+        const leftOffset = getLeftOffset(region.cols[0]);
+        nextLeft = getAdjustedScrollPosition(leftOffset, frozenColumnsCumulativeWidth);
+    } else {


this might make more sense as a switch? cuz that's exactly what it is, on enum values.

I like to avoid switches unless I can return immediately within each case; otherwise I have an extra break; line at the end of every case, which I find bloat-y and unsightly.

Also, TIL if you redeclare variables with the same name within different switch cases (e.g. topOffset and leftOffset), you get this compilation error: [ts] Cannot redeclare block-scoped variable 'leftOffset'.

you can always put your switch-case in a block

case 1: { const foo = 1; break; } case 2: { const foo = 2; break; }

breaks are annoying yes, but the benefit of compile-time warnings shouldn't be discarded.

also, you can refactor the switch to a function so you can use returns

@themadcreator nice, will change

giladgray · 2017-08-24T22:02:58Z

packages/table/src/common/internal/scrollUtils.ts

+function getAdjustedScrollPosition(scrollOffset: number, frozenRegionCumulativeSize: number) {
+    return scrollOffset < frozenRegionCumulativeSize
+        ? 0
+        : scrollOffset - frozenRegionCumulativeSize;


equivalent to Math.min(scrollOffset - frozenRegionCumulativeSize, 0), i think.

perhaps, instead of this function, you can just do the subtraction inline and the Math.min once when building the return object?

Yeah. It's a little clearer to me as is, but I can change it and just add a comment about the intent like // if the new scroll offset falls within the frozen region, clamp it to 0

EDIT: Math.max

giladgray · 2017-08-24T22:04:19Z

packages/table/src/quadrants/tableQuadrantStack.tsx

        if (this.wasMainQuadrantScrollChangedFromOtherOnWheelCallback) {
+            console.log("  this.wasMainQuadrantScrollChangedFromOtherOnWheelCallback == true.");
+            console.log("  Setting this.wasMainQuadrantScrollChangedFromOtherOnWheelCallback = false. RETURNING");


😱 🌳 logs 🌳 😱

giladgray · 2017-08-24T22:05:19Z

packages/table/src/table.tsx

+     *   Scroll the left-most column in the target region to the left side of the viewport.
+     *
+     *   FULL_TABLE:
+     *   Scroll the top-left cell to the top-left corner of the viewport.


can cut lines in a third by formatting as list.

- `CELLS`: words words - `FULL_ROWS`: more words

One of them was too long to fit and line-wrapped; this format takes more lines but keeps everything looking perfectly consistent. I'll see if I can whittle the words down and go back to a list.

blueprint-bot · 2017-08-25T18:02:58Z

Fix lint again

Preview: documentation | table
Coverage: core | datetime

tgreenwatts · 2017-08-25T20:48:29Z

seeing 1 small possible
-focus on c1 and then scroll to G, I lose my focus cell. It's selected but not focused?

cmslewis · 2017-08-25T21:09:18Z

@tgreenwatts I can't repro that. When I focus on C1 and then scrollTo column G, I still see the focus cell in C1 when I manually scroll back. What are the exact repro steps for the weirdness you're seeing?

blueprint-bot · 2017-08-25T21:11:39Z

Added docs, new 'Instance methods' section

Preview: documentation | table
Coverage: core | datetime

blueprint-bot · 2017-08-25T21:26:19Z

Update stuff per CR feedback

Preview: documentation | table
Coverage: core | datetime

llorca

👌

blueprint-bot · 2017-08-25T21:45:59Z

Oops, revert changes to table example

Preview: documentation | table
Coverage: core | datetime

themadcreator

baller tests. many nits, but nothing blocking.

themadcreator · 2017-08-25T21:36:52Z

packages/table/preview/index.tsx

+                return;
+        }
+
+        this.tableInstance.scrollToRegion(region);


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?

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

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;

themadcreator · 2017-08-25T21:37:52Z

packages/table/preview/index.tsx

@@ -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 }));


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

e.g. 4 lines above

themadcreator · 2017-08-25T21:40:02Z

packages/table/src/common/internal/scrollUtils.ts

@@ -0,0 +1,68 @@
+/**


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.

themadcreator · 2017-08-25T21:41:19Z

packages/table/src/common/internal/scrollUtils.ts

+    getTopOffset: (rowIndex: number) => number,
+    numFrozenRows: number = 0,
+    numFrozenColumns: number = 0,
+) {


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.

themadcreator · 2017-08-25T21:42:12Z

packages/table/src/common/internal/scrollUtils.ts

+    const frozenColumnsCumulativeWidth = getLeftOffset(numFrozenColumns);
+    const frozenRowsCumulativeHeight = getTopOffset(numFrozenRows);
+
+    if (cardinality === RegionCardinality.CELLS) {


for enums, i think we should use switch statements to make default: lintable.

Ah, good thought. Will change.

themadcreator · 2017-08-25T21:49:44Z

packages/table/src/table.tsx

@@ -435,6 +439,74 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
        };
    }

+    // Instance methods


nit: these sort of block comments indicate the file is too big and should be split. "nit" because i know that splitting is extremely difficult at this point.

Agree. I want to change this later (not this PR).

themadcreator · 2017-08-25T21:51:00Z

packages/table/src/table.tsx

+     * 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.
+     */


i wish there were some way to annotate these methods so we could easily use the jsdoc comment in our docs. appreciate your update the docs markdown though

themadcreator · 2017-08-25T21:51:39Z

packages/table/src/table.tsx

+     * corner is reached.
+     */
+    public scrollToRegion(region: IRegion) {
+        const { left: currScrollLeft, top: currScrollTop } = this.state.viewportRect;


holy crap, i didn't know you could rename with destructuring assignment!

Yeah I just learned this last month. Super nifty. 👍

themadcreator · 2017-08-25T21:52:16Z

packages/table/src/table.tsx

+            this.grid.getCumulativeHeightBefore,
+            numFrozenRows,
+            numFrozenColumns,
+        );


looks like passing the objects would only save a couple args =/

still, i prefer many args over jamming them into a custom argument object

themadcreator · 2017-08-25T21:54:36Z

packages/table/src/table.tsx

            if (didScrollTopChange) {
-                this.bodyElement.scrollTop = nextScrollTop;
+                const topCorrection = this.shouldDisableVerticalScroll() ? 0 : this.columnHeaderElement.clientHeight;


i think you use "adjustment" and "correction" interchangeably. choose one

cmslewis · 2017-08-26T21:07:42Z

@giladgray @themadcreator think I addressed all of your immediate comments.

blueprint-bot · 2017-08-26T21:14:30Z

Fix stuff per bdwyer CR

Preview: documentation | table
Coverage: core | datetime

Addressed all comments from before

blueprint-bot · 2017-08-28T21:21:47Z

Merge branch 'master' into cl/table-scroll-to-position

Preview: documentation | table
Coverage: core | datetime

cmslewis added 18 commits August 22, 2017 15:23

Delete commented code

f661262

Add method stub

bd91c72

Initial implementation

c4d9f6b

[TEMPORARY?] Add instance to the window object

9e694ab

Make auto-scrolling work; fix syncViewportPosition bug

70a8bb5

Handling for frozen rows/columns

bd1c170

Fix scroll-correction logic

f65c8f4

Add instance method to TableQuadrantStack

0ad4d8a

Refactor logic into new scrollUtils.ts

33bb63b

Create new common/internal folder

b4cf85e

Fix scroll misalignment bug

cc7b908

Don't correct if scroll is disabled

fcb8aed

Prevent programmatic scrolling when scrolling disabled

2c29b21

Remove need for Grid in scrollUtils file

a29f832

Fix off-by-one bug

a87242d

Add scrollTo controls to the table example

5cbf3b6

Fix lint

ca9be6b

Delete animated param for now

f5080e4

cmslewis added the Package: table label Aug 24, 2017

cmslewis requested review from llorca, giladgray, themadcreator, gscshoyru and tgreenwatts August 24, 2017 20:11

Delete window instance

28fcb52

cmslewis commented Aug 24, 2017

View reviewed changes

cmslewis added 3 commits August 24, 2017 13:20

Delete console.logs

9d1d9f1

Revert changes RE: prereqStateKeyValue

5281104

Delete unintentional cnewline

839c027

giladgray previously requested changes Aug 24, 2017

View reviewed changes

cmslewis added 2 commits August 24, 2017 16:02

Write tests for scrollUtils

89b7b05

Fix lint again

da186b5

Write tests in table too

dfb5a9c

cmslewis force-pushed the cl/table-scroll-to-position branch from a01046f to dfb5a9c Compare August 25, 2017 19:47

Added docs, new 'Instance methods' section

26b4fc8

Update stuff per CR feedback

70ca679

Oops, revert changes to table example

f21b683

llorca approved these changes Aug 25, 2017

View reviewed changes

themadcreator approved these changes Aug 25, 2017

View reviewed changes

cmslewis added Type: enhancement Domain: API P1 labels Aug 25, 2017

Fix stuff per bdwyer CR

ec3e915

gscshoyru approved these changes Aug 28, 2017

View reviewed changes

Merge branch 'master' into cl/table-scroll-to-position

80dfb99

tgreenwatts approved these changes Aug 28, 2017

View reviewed changes

cmslewis merged commit 95d431d into master Aug 28, 2017

cmslewis deleted the cl/table-scroll-to-position branch August 28, 2017 21:26

cmslewis mentioned this pull request Aug 30, 2017

Prepare Release 1.27.0 #1513

Merged

[Table] scrollToRegion instance method #1496

[Table] scrollToRegion instance method #1496

Conversation

cmslewis commented Aug 24, 2017 • edited by themadcreator Loading

Fixes #1065, Fixes #1430

Checklist

Changes proposed in this pull request:

Followup work (for later)

Reviewers should focus on:

Screenshot

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

cmslewis Aug 26, 2017 • edited Loading

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 Aug 26, 2017 • edited Loading

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 Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Aug 25, 2017 • edited Loading

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

blueprint-bot commented Aug 25, 2017

Fix lint again

tgreenwatts commented Aug 25, 2017

cmslewis commented Aug 25, 2017 • edited Loading

blueprint-bot commented Aug 25, 2017

Added docs, new 'Instance methods' section

blueprint-bot commented Aug 25, 2017

Update stuff per CR feedback

llorca left a comment

Choose a reason for hiding this comment

blueprint-bot commented Aug 25, 2017

Oops, revert changes to table example

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

cmslewis Aug 26, 2017 • edited Loading

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

cmslewis Aug 26, 2017 • edited Loading

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 Aug 24, 2017 •

edited by themadcreator

Loading

cmslewis Aug 26, 2017 •

edited

Loading

cmslewis Aug 26, 2017 •

edited

Loading

themadcreator Aug 25, 2017 •

edited

Loading

cmslewis Aug 25, 2017 •

edited

Loading

cmslewis commented Aug 25, 2017 •

edited

Loading

cmslewis Aug 26, 2017 •

edited

Loading

cmslewis Aug 26, 2017 •

edited

Loading