Skip to content

Commit

Permalink
[PopOver] Fixed bug where PopOver renders relative to screen rather t…
Browse files Browse the repository at this point in the history
…han layout container in React Fiber (#7663)

* replaced all occurances of onTouchTap with onClick

* changed tests to use 'click' event, and seem to all be passing.

* [eslint] Alphabetically ordered onClick in props for code quality

* [Popover] Added 1ms timeout to render popover to ensure placement is created correctly, after [RenderToLayer] has completed it's render

* [Popover] removed unused development prop on [RenderToLayer]

* [PopOver] Fixed tests to be inline with the new behaviour (waiting for layout to be setup correctly). Also fixed [Menu] tests to work with recent onTouchTap -> onClick changes.

* [PopOver] cleared timeout on unmount and simplified timeout without delay
  • Loading branch information
lostpebble authored and oliviertassinari committed Aug 5, 2017
1 parent a96ddc1 commit 8432807
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 10 deletions.
4 changes: 2 additions & 2 deletions src/Menu/Menu.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,12 +269,12 @@ describe('<Menu />', () => {
</Menu>
);

wrapper.find('.item1').simulate('touchTap');
wrapper.find('.item1').simulate('click');
assert.strictEqual(wrapper.state('focusIndex'), 0);
document.body.dispatchEvent(new window.Event('mouseup', {bubbles: true}));
assert.strictEqual(wrapper.state('focusIndex'), -1);

wrapper.find('.item2').simulate('touchTap');
wrapper.find('.item2').simulate('click');
assert.strictEqual(wrapper.state('focusIndex'), 1);
document.body.dispatchEvent(new window.Event('mouseup', {bubbles: true}));
assert.strictEqual(wrapper.state('focusIndex'), 1);
Expand Down
18 changes: 13 additions & 5 deletions src/Popover/Popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,16 @@ class Popover extends Component {
this.handleResize = throttle(this.setPlacement, 100);
this.handleScroll = throttle(this.setPlacement.bind(this, true), 50);

this.popoverRefs = {};

this.state = {
open: props.open,
closing: false,
};
}

componentDidMount() {
this.setPlacement();
this.placementTimeout = setTimeout(this.setPlacement);
}

componentWillReceiveProps(nextProps) {
Expand Down Expand Up @@ -173,13 +175,19 @@ class Popover extends Component {
}

componentDidUpdate() {
this.setPlacement();
clearTimeout(this.placementTimeout);
this.placementTimeout = setTimeout(this.setPlacement);
}

componentWillUnmount() {
this.handleResize.cancel();
this.handleScroll.cancel();

if (this.placementTimeout) {
clearTimeout(this.placementTimeout);
this.placementTimeout = null;
}

if (this.timeout) {
clearTimeout(this.timeout);
this.timeout = null;
Expand Down Expand Up @@ -285,11 +293,11 @@ class Popover extends Component {
return;
}

if (!this.refs.layer.getLayer()) {
if (!this.popoverRefs.layer.getLayer()) {
return;
}

const targetEl = this.refs.layer.getLayer().children[0];
const targetEl = this.popoverRefs.layer.getLayer().children[0];
if (!targetEl) {
return;
}
Expand Down Expand Up @@ -410,7 +418,7 @@ class Popover extends Component {
onResize={this.handleResize}
/>
<RenderToLayer
ref="layer"
ref={(ref) => this.popoverRefs.layer = ref}
open={this.state.open}
componentClickAway={this.componentClickAway}
useLayerForClickAway={this.props.useLayerForClickAway}
Expand Down
9 changes: 6 additions & 3 deletions src/Popover/Popover.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ describe('<Popover />', () => {
it('should stop listening correctly', (done) => {
const wrapper = mountWithContext(<Popover open={true} />);

wrapper.instance().handleScroll();
wrapper.instance().handleScroll();
wrapper.unmount();
// Ensure layering has been set up correctly before simulation
setTimeout(() => {
wrapper.instance().handleScroll();
wrapper.instance().handleScroll();
wrapper.unmount();
}, 10);

setTimeout(() => {
// Wait for the end of the throttle. Makes sure we don't crash.
Expand Down

0 comments on commit 8432807

Please sign in to comment.