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] Fixed bug where Popover renders relative to screen #7663

Merged
merged 8 commits into from
Aug 5, 2017
Merged

[Popover] Fixed bug where Popover renders relative to screen #7663

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4dfa533
replaced all occurances of onTouchTap with onClick
lostpebble Aug 1, 2017
2e34d9e
changed tests to use 'click' event, and seem to all be passing.
lostpebble Aug 1, 2017
90d92ee
[eslint] Alphabetically ordered onClick in props for code quality
lostpebble Aug 2, 2017
ca7f833
[Popover] Added 1ms timeout to render popover to ensure placement is …
lostpebble Aug 2, 2017
eeb0486
resolved merge conflict
lostpebble Aug 4, 2017
c1a1091
[Popover] removed unused development prop on [RenderToLayer]
lostpebble Aug 4, 2017
6c3814c
[PopOver] Fixed tests to be inline with the new behaviour (waiting fo…
lostpebble Aug 4, 2017
b4fffa8
[PopOver] cleared timeout on unmount and simplified timeout without d…
lostpebble Aug 5, 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
4 changes: 2 additions & 2 deletions src/Menu/Menu.spec.js
View file
Open in desktop
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
12 changes: 7 additions & 5 deletions src/Popover/Popover.js
View file
Open in desktop
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();
setTimeout(this.setPlacement, 1);
Copy link
Member

@oliviertassinari oliviertassinari Aug 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That's hidden something, I would normally ask to look into it, we shouldn't have such code. But anyway, the master is no longer a priority. Also, that might lead to other potential edge cases.

Can we just clear the timeout when unmounting? Also, no need to set 1, 0 or no argument is enough.

Copy link
Author

@lostpebble lostpebble Aug 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Okay cool, I'll make those changes and do the clear on unmount. Just figured it would be over so quick that clearing it is irrelevant - but I guess these things all work on a much more micro scale.

}

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

componentDidUpdate() {
this.setPlacement();
setTimeout(this.setPlacement, 1);
}

componentWillUnmount() {
Expand Down Expand Up @@ -285,11 +287,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 +412,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
View file
Open in desktop
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