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.

Sign up for GitHub

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

[Popover] Scroll Container issue #7472

Merged
merged 9 commits into from
Jul 21, 2017
+12 −3
Merged

[Popover] Scroll Container issue #7472

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fea465e
#4783
Jul 19, 2017
14c5824
Update Popover.js
oliviertassinari Jul 20, 2017
629307e
-Commented PR changes are applied
Jul 21, 2017
d0a9a75
Merge branch '4783-AutoCompletePopoverScrollIssue' of https://github.…
Jul 21, 2017
553024b
Update Popover.js
oliviertassinari Jul 21, 2017
1b9e3a7
Prop order is changed as alphabetical order
Jul 21, 2017
75799de
Merge branch '4783-AutoCompletePopoverScrollIssue' of https://github.…
Jul 21, 2017
9dbdb43
redundant space removed
Jul 21, 2017
50b7c83
redundant space is removed
Jul 21, 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
13 changes: 10 additions & 3 deletions src/Popover/Popover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ class Popover extends Component {
* The zDepth of the popover.
*/
zDepth: propTypes.zDepth,
/**
* TODO
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a description for the documentation?

*/
scrollElement: PropTypes.oneOfType([
PropTypes.object,
PropTypes.string,
]),
};

static defaultProps = {
Expand Down Expand Up @@ -304,8 +311,8 @@ class Popover extends Component {
targetPosition = this.applyAutoPositionIfNeeded(anchor, target, targetOrigin, anchorOrigin, targetPosition);
}

targetEl.style.top = `${Math.max(0, targetPosition.top)}px`;
targetEl.style.left = `${Math.max(0, targetPosition.left)}px`;
targetEl.style.top = `${targetPosition.top}px`;
targetEl.style.left = `${targetPosition.left}px`;
Copy link

Choose a reason for hiding this comment

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

This change appears to be breaking large lists where the scrolling container (e.g. window) doesn't actually scroll. Previously prior to this change, having the 'Math.max' bit allowed the popover content to scroll rather than the scrolling container, but by allowing the popover to now extend beyond the bounds of the target element, this is broken. Could we perhaps have a new prop to bring back clamping to 0?

Copy link

Choose a reason for hiding this comment

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

I have created issue #7573 for this.

Copy link
Member

Choose a reason for hiding this comment

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

@jamespizzurro Oh, we can always revert. I think that we will soon freeze the master branch. Fixes have been introducing too many regressions lately, the master doesn't have enough tests.

targetEl.style.maxHeight = `${window.innerHeight}px`;
};

Expand Down Expand Up @@ -395,7 +402,7 @@ class Popover extends Component {
return (
<div style={styles.root}>
<EventListener
target="window"
target={this.props.scrollElement || "window"}
Copy link
Member

Choose a reason for hiding this comment

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

What about using a defaultProps instead?

onScroll={this.handleScroll}
onResize={this.handleResize}
/>
Expand Down