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

[new] ResizeSensor component #2745

Merged
merged 12 commits into from
Aug 1, 2018
Merged

[new] ResizeSensor component #2745

merged 12 commits into from
Aug 1, 2018

Conversation

giladgray
Copy link
Contributor

  • <ResizeSensor> provides a React interface to ResizeObserver with a required onResize callback.
  • refactor OverflowList and Popover to use this new guy.

@blueprint-bot
Copy link

don't expose external types

Preview: documentation | landing | table

@@ -350,7 +335,11 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
disabled: isOpen && Utils.isElementOfType(rawTarget, Tooltip) ? true : rawTarget.props.disabled,
tabIndex: this.props.openOnTargetFocus && isHoverInteractionKind ? tabIndex : undefined,
});
return React.createElement(targetTagName, targetProps, clonedTarget);
return (
<ResizeSensor onResize={this.handlePopoverResize}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is new and awesome and so simple. measures the target too!

"es5",
"es2015.collection",
"es2015.iterable",
"es2015.promise"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added Promise types to tests to simplify the nested timeouts


function resize(size: ISizeProps) {
wrapper.setProps(size);
return new Promise(resolve => setTimeout(resolve, 30));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sooo resizes need time to propagate (more than a frame, typically), so we must wrap each one in this brief timeout. fortunately, async/await make this rather nice to use instead of callback hell.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh.... this seems like a pretty big gotcha

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is going to be relevant to users of this component, you should add a comment to the onResize prop.

@blueprint-bot
Copy link

add docs page

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix isotest

Preview: documentation | landing | table

Copy link
Member

@invliD invliD left a comment

Choose a reason for hiding this comment

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

I love it!

/**
* If `true`, all parent DOM elements of the container will also be
* observed. If changes to a parent's size is detected, the overflow will be
* recalculated.
Copy link
Member

Choose a reason for hiding this comment

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

the overflow part doesn't belong here

*
* Only enable this prop if the overflow should be recalculated when a
* parent element resizes in a way that does not also cause the
* `OverflowList` to resize.
Copy link
Member

Choose a reason for hiding this comment

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

same here, update docs

}

export class ResizeSensor extends React.PureComponent<IResizeSensorProps> {
public static displayName = `${DISPLAYNAME_PREFIX}.Resize`;
Copy link
Member

Choose a reason for hiding this comment

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

ResizeSensor

document.documentElement.appendChild(testsContainerElement);

afterEach(() => {
// clean up list after each test, if it was used
Copy link
Member

Choose a reason for hiding this comment

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

not a list anymore

@blueprint-bot
Copy link

docs per comments

Preview: documentation | landing | table


@# Resize sensor

`ResizeSensor` is a higher-order component that effectively provides a
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is a little wordy and im not sure it's even correct. Maybe rephrase:

`ResizeSensor` is a component that provides a `"resize"` event for its single child.

But even then it's not "providing an event" in the sense of .addEventListener.


function resize(size: ISizeProps) {
wrapper.setProps(size);
return new Promise(resolve => setTimeout(resolve, 30));
Copy link
Contributor

Choose a reason for hiding this comment

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

oh.... this seems like a pretty big gotcha


function resize(size: ISizeProps) {
wrapper.setProps(size);
return new Promise(resolve => setTimeout(resolve, 30));
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is going to be relevant to users of this component, you should add a comment to the onResize prop.

@blueprint-bot
Copy link

docs cleanup

Preview: documentation | landing | table

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.

4 participants