[OverflowList] add onOverflow callback prop #2975

giladgray · 2018-09-26T18:28:33Z

Changes proposed in this pull request:

add OverflowList onOverflow callback prop which receives the overflowed items.
invoked at most once per resize and only if the overflowed items actually changed.
- this was tricky to support but i got it working nicely!

Reviewers should focus on:

reliability of this code change.

Screenshot

added some state fields to support this logic. complex stuff as it can happen over several renders.

giladgray · 2018-09-26T18:30:11Z

packages/core/src/components/overflow-list/overflowList.tsx

 import { IResizeEntry, ResizeSensor } from "../resize-sensor/resizeSensor";

+/** @internal - do not expose this type */


thoughts on doing this to reduce the exported surface area? it excludes it from the .d.ts files and required marking the state field similarly (line 84).

blueprint-bot · 2018-09-26T18:30:41Z

more onOverflow tests & helper

Previews: documentation | landing | table

invliD · 2018-09-26T18:45:10Z

packages/core/src/components/overflow-list/overflowList.tsx

        this.repartition(false);
+        if (this.state.direction === OverflowDirection.NONE && this.state.direction !== prevState.direction) {
+            if (


this could be just one if

packages/core/src/components/overflow-list/overflowList.tsx

invliD · 2018-09-26T18:50:12Z

packages/core/src/components/overflow-list/overflowList.tsx

+     */
+    direction: OverflowDirection;
+    /** Remember the last completed overflow to dedupe `onOverflow` calls during smooth resizing. */
+    lastOverflow?: T[];


if you make this non-optional you have one fewer check to make below. You don't actually treat the undefined case in a special way.

undefined is different from empty list. the distinction is actually useful.

packages/core/src/components/overflow-list/overflowList.tsx

blueprint-bot · 2018-09-26T20:43:16Z

more docs, combine if

Previews: documentation | landing | table

blueprint-bot · 2018-09-26T20:43:20Z

more docs, combine if

Previews: documentation | landing | table

blueprint-bot · 2018-09-26T20:43:23Z

more docs, combine if

Previews: documentation | landing | table

blueprint-bot · 2018-09-26T21:05:44Z

reset lastOverflow with new props

Previews: documentation | landing | table

cmslewis

@giladgray

packages/core/src/components/overflow-list/overflowList.tsx

cmslewis · 2018-09-27T00:02:41Z

packages/core/src/components/overflow-list/overflowList.tsx

+            // shrink operation does not require checking last items
+            (prevState.direction === OverflowDirection.SHRINK ||
+                // if current overflow is different from last completed state
+                this.state.lastOverflow == null ||


Could we do === undefined, or is == null just idiomatic for the codebase?

cmslewis · 2018-09-27T00:04:34Z

packages/core/src/components/overflow-list/overflowList.tsx

+            (prevState.direction === OverflowDirection.SHRINK ||
+                // if current overflow is different from last completed state
+                this.state.lastOverflow == null ||
+                this.state.lastOverflow.some((x, i) => x !== this.state.overflow[i]))


Could we just check lastOverflow.length !== overflow.length to save a little time? Is it possible for a change in overflow to replace items without changing the number of items displayed?

yes it's completely possible for that to happen, though that should be handled by the check in componentWillReceiveProps. so maybe this check can be made simpler. i'll play with it some more.

worked flawlessly!

cmslewis · 2018-09-27T00:05:59Z

packages/core/src/components/overflow-list/overflowList.tsx

 import { IResizeEntry, ResizeSensor } from "../resize-sensor/resizeSensor";

+/** @internal - do not expose this type */


…low-list-event

blueprint-bot · 2018-09-27T21:23:25Z

simplify lastOverflow check - just length

Previews: documentation | landing | table

invliD · 2018-10-02T17:54:51Z

packages/core/src/components/overflow-list/overflowList.tsx

+            // if a resize operation has just completed (transition to NONE)
+            direction === OverflowDirection.NONE &&
+            direction !== prevState.direction &&
+            (lastOverflowCount == null || overflow.length !== lastOverflowCount)


this is never set to null or undefined, so the first part will never be true

oh yeah i removed the nullability!

invliD · 2018-10-02T17:57:17Z

packages/core/src/components/overflow-list/overflowList.tsx

@@ -85,6 +107,8 @@ export class OverflowList<T> extends React.PureComponent<IOverflowListProps<T>,
    }

    public state: IOverflowListState<T> = {
+        direction: OverflowDirection.NONE,
+        lastOverflowCount: -1,


Why -1? The overflow initially has 0 elements.

invliD · 2018-10-02T18:00:18Z

packages/core/src/components/overflow-list/overflowList.tsx

@@ -85,6 +107,8 @@ export class OverflowList<T> extends React.PureComponent<IOverflowListProps<T>,
    }

    public state: IOverflowListState<T> = {
+        direction: OverflowDirection.NONE,


We initially call repartition in shrinking mode, so maybe this should be SHRINK (if you want onOverflow to be called initially, even when the overflow is empty. If you don't, just keep NONE)

i changed to SHRINK but it was not called initially in tests, so just sticking with this.

blueprint-bot · 2018-10-02T18:57:29Z

lastOverflowCount defaults to 0

Previews: documentation | landing | table

invliD

❤️

invliD · 2018-10-02T23:56:33Z

packages/core/src/components/overflow-list/overflowList.tsx

-            this.setState({
+            this.setState(state => ({
+                direction: OverflowDirection.GROW,
+                // store last overflow if this is the beginning of a resize (for check in `update()`).


can you call this componentDidUpdate or remove the backticks and parens so it's clearer what you mean?

blueprint-bot · 2018-10-04T19:02:23Z

fix name in comment

Previews: documentation | landing | table

blueprint-bot · 2018-10-04T19:02:25Z

fix name in comment

Previews: documentation | landing | table

Gilad Gray added 5 commits September 25, 2018 17:25

complete refactor to tests with a sweet harness

38fdb92

add onOverflow callback prop & isResizing state & test.

3702889

short example crumbs so they can all be visible

b4c3d4e

only invoke onOverflow when items change

a335135

added some state fields to support this logic. complex stuff as it can happen over several renders.

more onOverflow tests & helper

52a45f1

giladgray requested a review from invliD September 26, 2018 18:28

giladgray commented Sep 26, 2018

View reviewed changes

invliD reviewed Sep 26, 2018

View reviewed changes

more docs, combine if

b243222

reset lastOverflow with new props

51f4d26

cmslewis reviewed Sep 27, 2018

View reviewed changes

giladgray mentioned this pull request Sep 27, 2018

[OverflowLists] refactor tests to use a sweet harness #2981

Merged

Gilad Gray added 2 commits September 27, 2018 14:04

Merge branch 'develop' of github.com:palantir/blueprint into gg/overf…

f6ee5cc

…low-list-event

simplify lastOverflow check - just length

4b14ed4

giladgray requested review from invliD and cmslewis September 28, 2018 18:12

cmslewis approved these changes Oct 1, 2018

View reviewed changes

invliD reviewed Oct 2, 2018

View reviewed changes

lastOverflowCount defaults to 0

5f0aa57

invliD approved these changes Oct 2, 2018

View reviewed changes

fix name in comment

17e4af9

giladgray merged commit 8f86ea9 into develop Oct 4, 2018

giladgray deleted the gg/overflow-list-event branch October 4, 2018 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OverflowList] add onOverflow callback prop #2975

[OverflowList] add onOverflow callback prop #2975

giladgray commented Sep 26, 2018 •

edited

Loading

giladgray Sep 26, 2018

cmslewis Sep 27, 2018

blueprint-bot commented Sep 26, 2018

invliD Sep 26, 2018

invliD Sep 26, 2018

giladgray Sep 26, 2018

blueprint-bot commented Sep 26, 2018

blueprint-bot commented Sep 26, 2018

blueprint-bot commented Sep 26, 2018

blueprint-bot commented Sep 26, 2018

cmslewis left a comment

cmslewis Sep 27, 2018

giladgray Sep 27, 2018

cmslewis Sep 27, 2018

giladgray Sep 27, 2018

giladgray Sep 27, 2018

cmslewis Sep 27, 2018

blueprint-bot commented Sep 27, 2018

invliD Oct 2, 2018

giladgray Oct 2, 2018

invliD Oct 2, 2018

invliD Oct 2, 2018

giladgray Oct 2, 2018

blueprint-bot commented Oct 2, 2018

invliD left a comment

invliD Oct 2, 2018

blueprint-bot commented Oct 4, 2018

blueprint-bot commented Oct 4, 2018

		import { IResizeEntry, ResizeSensor } from "../resize-sensor/resizeSensor";

		/** @internal - do not expose this type */

[OverflowList] add onOverflow callback prop #2975

[OverflowList] add onOverflow callback prop #2975

Conversation

giladgray commented Sep 26, 2018 • edited Loading

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 26, 2018

more onOverflow tests & helper

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 26, 2018

more docs, combine if

blueprint-bot commented Sep 26, 2018

more docs, combine if

blueprint-bot commented Sep 26, 2018

more docs, combine if

blueprint-bot commented Sep 26, 2018

reset lastOverflow with new props

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

blueprint-bot commented Sep 27, 2018

simplify lastOverflow check - just length

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 Oct 2, 2018

lastOverflowCount defaults to 0

invliD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Oct 4, 2018

fix name in comment

blueprint-bot commented Oct 4, 2018

fix name in comment

giladgray commented Sep 26, 2018 •

edited

Loading