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

[Core] Add overflow list component #2537

Merged
merged 30 commits into from
Jun 1, 2018
Merged

[Core] Add overflow list component #2537

merged 30 commits into from
Jun 1, 2018

Conversation

invliD
Copy link
Member

@invliD invliD commented May 22, 2018

This is good stuff. Check it out.
No tests or documentation yet, just an example.

@blueprint-bot
Copy link

Add overflow list component

Preview: documentation | landing | table

@giladgray
Copy link
Contributor

giladgray commented May 22, 2018

need to add an entry in core/test/isotest.js

@giladgray giladgray self-requested a review May 22, 2018 20:00
@blueprint-bot
Copy link

Refactor throttle

Preview: documentation | landing | table

@blueprint-bot
Copy link

Add iso test props

Preview: documentation | landing | table

/** Throttle a method by wrapping it in a `requestAnimationFrame` call. */
// tslint:disable-next-line:ban-types
export function throttle<T extends Function>(method: T): T {
return (_throttleHelper(undefined, undefined, method as any) as any) as T;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we fix the types of _throttleHelper while we're at this?

display: flex;
min-width: 0;

.#{$ns}-overflow-list-spacer {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't need to be nested, the classname is specific enough

visible: props.items,
};

this.observer = new ResizeObserver(this.handleResize);
Copy link
Contributor

Choose a reason for hiding this comment

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

this constructor is entirely unnecessary. just assign public state = { overflow: [], visible: this.props.items } and initialize observer when defining it.

/**
* Callback invoked to render the overflow if necessary.
*/
overflowRenderer: (items: T[]) => React.ReactNode;
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 overflowItems to clarify that this is a subset of `items prop?

} else if (this.spacer.getBoundingClientRect().width < 1) {
this.setState(state => {
const visible = state.visible.slice();
const next = this.props.collapseFrom === Boundary.START ? visible.shift() : visible.pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

local var for this.props.collapseFrom === Boundary.START for legibility


public componentDidMount() {
if (this.element != null) {
this.observer.observe(this.element);
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment that observer invokes callback immediately to start

Copy link
Contributor

Choose a reason for hiding this comment

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

also need to invoke this.resize() on mount for first frame

public componentDidMount() {
if (this.element != null) {
this.observer.observe(this.element);
this.previousWidths.set(this.element, this.element.getBoundingClientRect().width);
Copy link
Contributor

Choose a reason for hiding this comment

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

are these necessary?

@blueprint-bot
Copy link

Unnest spacer css

Preview: documentation | landing | table

@llorca llorca changed the title Add overflow list component [Core] Add overflow list component May 22, 2018
@@ -153,6 +153,9 @@ export const NON_IDEAL_STATE_VISUAL = `${NON_IDEAL_STATE}-visual`;

export const NUMERIC_INPUT = `${NS}-numeric-input`;

export const OVERFLOW_LIST = `${NS}-overflow-list`;
export const OVERFLOW_LIST_SPACER = `${NS}-overflow-list-spacer`;
Copy link
Contributor

Choose a reason for hiding this comment

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

${OVERFLOW_LIST}-spacer

@blueprint-bot
Copy link

Remove constructor

Preview: documentation | landing | table

@blueprint-bot
Copy link

Clarify overflow

Preview: documentation | landing | table

@blueprint-bot
Copy link

Improve legibility of repartition method

Preview: documentation | landing | table

if (props.href != null) {
return (
<li key={index}>
<a className={Classes.BREADCRUMB}>{props.text}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

can't use <Breadcrumb>?

Copy link
Contributor

Choose a reason for hiding this comment

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

hah yeah you could

@blueprint-bot
Copy link

Remove extraneous element measuring

Preview: documentation | landing | table

@blueprint-bot
Copy link

Improve classes const definition

Preview: documentation | landing | table

@blueprint-bot
Copy link

Stop measuring all elements on mount

Preview: documentation | landing | table

@blueprint-bot
Copy link

stuff

Preview: documentation | landing | table

@blueprint-bot
Copy link

Update _overflow-list.scss

Preview: documentation | landing | table

@@ -1,6 +1,6 @@
@# Overflow list

this is cool
`OverflowList` displays a list of items that can overflow on its own. The required `visibleItemRenderer` callback prop allows for customizing the appearance of visible items. The required `overflowRenderer` callback prop allows for customizing the rendering of collapsed items.
Copy link
Contributor

Choose a reason for hiding this comment

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

"on its own" means nothing. we should clarify what causes overflowing -- specifically, resize.

@blueprint-bot
Copy link

Merge branch 'develop' into sb/overflow-list

Preview: documentation | landing | table

@giladgray
Copy link
Contributor

@invliD i'm rerunning the test-react-15 build to see if these are flakes

@giladgray
Copy link
Contributor

this could be useful... https://mochajs.org/#retry-tests

@blueprint-bot
Copy link

docs docs docs!

Preview: documentation | landing | table

@@ -1,6 +1,19 @@
@# Overflow list

`OverflowList` displays a list of items that can overflow on its own. The required `visibleItemRenderer` callback prop allows for customizing the appearance of visible items. The required `overflowRenderer` callback prop allows for customizing the rendering of collapsed items.
`OverflowList` takes a generic list of items and renders as many items as can
fit inside itself. Other items are collapsed into an overflow menu. The visible
Copy link
Contributor

Choose a reason for hiding this comment

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

Other items are collapsed into an overflow menu.

That's quite opinionated, I preferred the more generic previous wording. Can you replace overflow menu with overflow element?

The `items` prop accepts an array of generic objects. The required
`visibleItemRenderer` callback prop determines the appearance of a visible item.
The required `overflowRenderer` callback prop receives all overflowed items
and renders the overflow menu or indicator.
Copy link
Contributor

Choose a reason for hiding this comment

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

and allows customizing the rendering of overflown items?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not custom though, it's required

Copy link
Contributor

@llorca llorca May 31, 2018

Choose a reason for hiding this comment

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

That's not what I mean. The prop is required yes, but rendering an overflow menu or indicator isn't required. I can render a menu, a static icon, or absolutely nothing if I want to--that's what's powerful

* @param overflowItems The items that didn’t fit in the container.
* Callback invoked to render the overflowed items. The function will
* receive all items that do not fit in the container and should render a
* container for these items.
Copy link
Contributor

Choose a reason for hiding this comment

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

would drop and should render a container for these items. or move it to typical use cases below

@blueprint-bot
Copy link

revert constructor and clientWidth

Preview: documentation | landing | table

@blueprint-bot
Copy link

docs edits

Preview: documentation | landing | table

@blueprint-bot
Copy link

Update overflow-list.md

Preview: documentation | landing | table

@giladgray giladgray merged commit 3d461e8 into develop Jun 1, 2018
@giladgray giladgray deleted the sb/overflow-list branch June 1, 2018 01:43
@giladgray giladgray mentioned this pull request Jun 1, 2018
@qcharlieshi
Copy link

qcharlieshi commented Jun 6, 2018

Will this be merged back into the earlier versions such as 1.35 eventually? I would be willing to help out with that if needed

@invliD
Copy link
Member Author

invliD commented Jun 6, 2018

We consider this somewhat of a break due to the new required dependency on the resize observer polyfill, which is why we previously decided not to backport. I don't feel strongly either way. @giladgray?

@qcharlieshi
Copy link

Hmm, let me know if y'all are willing to merge it back, I'm happy to contribute if the maintainers feel like its not worth the effort. It'll be awhile before my project is upgraded to react 16 and then the subsequent blueprint versions.

As long as its not a huge issue to backport it.

@giladgray
Copy link
Contributor

We do not back port features, ever. Only forward-porting.

Blueprint 3.0 (@next on npm) supports React 15 & 16, so nothing is stopping you @qcharlieshi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants